Repository navigation
fix(task-orphan-check): age a task from when it entered this inbox, not when it was sent [AgentActivity] - #5929
Conversation
…ot when it was sent [AgentActivity] classify.py aged a task from its `timestamp:` header, the sender's send time. A message held at the broker while the app was closed for more than 5 min was therefore an orphan the moment it arrived, and the startup pass archived it unanswered. Age now comes from `local_time:`, which every task writer stamps when it writes the file; `timestamp:`, the id's epoch-ms and the mtime remain the fallbacks in that order.
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 94%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
…-from-inbox # Conflicts: # CHANGELOG-PENDING.md
|
@cla-assistant check |
sonichi
left a comment
There was a problem hiding this comment.
Approving. Preferring local_time: (written by the local writer at intake, carried inside the file so copies and moves keep it) over the sender's timestamp is the right anchor for "how long has this sat in my inbox". Redelivery of a still-pending task only fsyncs (_repair_pending_task) and doesn't restamp, so a stale task can't be made to look fresh. Zero-byte deliveries/ markers never feed the age, because a held task returns before age is computed. tests/task-orphan-check-classify.test.py passes locally (66/66) and CI is green.
Wording nits:
- "Every writer stamps its own write time" is overstated.
task-bridge.tsbuildslocal_timefrom the sender's timestamp, andbuild_requeued_task(src/result_markers.py) copies the originallocal_timeinto a requeued task. Behaviour is unchanged from before, but SKILL.md and the docstring should say so. status_ack_policy.intake_atstill dates by the sender'stimestamp. That's defensible for an ack deadline, but worth a line saying the split is intended.
(agent-assisted, Blue for Chi's seat)
keweichen
left a comment
There was a problem hiding this comment.
Changes requested at 3d1dcc071fac352c977a6dbbeb2580e087d0ad5f — not ready to merge.
Blocker 1 — a legacy task body can forge the newly trusted age source. classify_task() uses parse_task_headers_lenient() at skills/task-orphan-check/scripts/classify.py:233-240, then task_queued_at() trusts headers["local_time"] at lines 159-164. The parser's own contract at src/local_task_protocol.py:384-395 says that when an older writer legitimately lacks a key, a body line can supply it.
Exact-head repro with an authentic 12-second-old timestamp: and an old local_time: line inside the user body:
lenient_local_time 2019-10-30T00:00:00+00:00 UTC
age_from local_time
age_seconds 220924800
strict_has_local_time False
The full classifier changes this fresh legacy task from fresh on the parent to orphan at HEAD. Startup then archives it unanswered. This is the rolling-upgrade case the review criteria require: current writers stamp/defang the header, but older task shapes do not. Please consume local_time only when its pre-task: header provenance is established, and add the negative regression.
Blocker 2 — the startup live witness is still missing. The PR's Live section is a future test recipe, not a completed post-restart round trip. Per REVIEW.md lesson 15, approval needs the real quit / broker-held message / >5-minute wait / relaunch path showing the message is answered and not archived.
I ran the mandatory preflight, all 66 classifier tests, the local-time suite, a parent negative control, and the review scanner; they otherwise pass, and CI/CLA are green.
liususan091219
left a comment
There was a problem hiding this comment.
Approving. The premise is the part worth checking, and it holds: local_time: is stamped at write time by every writer that emits it, so it really is "how long this task has waited in this inbox".
I audited the writers rather than taking the header's name on faith, because the fix is only correct if none of them derives local_time from the sender's clock:
packages/ag2-sparrow/ag2_sparrow/remote_gateway_bridge.py:4147andevent_consumer.py:140—local_time_value()with no argument, i.e. the current instant. This is the ag2space path, the one the held-at-broker scenario actually travels.slack-bridge.py:1271,discord-bridge.py:4279,telegram-bridge.py:1024— same no-argument call.task-bridge.ts:157and:920pass a timestamp, which initially looked like the sender's clock. It isn't: at:157timestampisnew Date().toISOString()on the line above, and:920isbuildVoiceTaskHeaderfor locally-originated voice tasks. Both are write time.
So no writer feeds a sender-side instant into this field, and the new first preference is sound.
Two notes, neither blocking:
- A task with no
local_time:falls through totimestamp:and behaves exactly as before, so older/other writers degrade gracefully. - If
local_timeis ever in the future (clock skew), the age goes negative and the task reads FRESH — it stays intasks/rather than being archived. That is the fail-safe direction for this classifier, so I'd leave it, but it is worth knowing it fails open rather than closed.
The test earns its place: it reproduces the actual failure (old sender timestamp, recent local_time, source: ag2space) rather than asserting the new code path exists, and it uses a non-UTC offset so a naive parse would show up.
|
Post-merge timing note: GitHub records this PR merged at The two findings therefore did not gate this merge and remain actionable on Please address the parser-provenance regression before the next release. |
liususan091219
left a comment
There was a problem hiding this comment.
Withdrawing my approval above. @keweichen's Blocker 1 is correct and I verified it independently; my review missed it.
parse_task_headers_lenient's own contract says so in as many words (src/local_task_protocol.py:384-395):
First-wins resists trailing-body forgery when the real header exists; it does not protect a file that legitimately lacks the probed key (a body line can then supply it).
So for any task shape that does not stamp local_time: — exactly the older writers this fallback chain exists to serve — a local_time: line sitting in the user-supplied body becomes the trusted age source, ahead of the authentic timestamp:. A fresh task then reads as an orphan and is archived unanswered, which is the specific harm this classifier exists to prevent.
Worth naming why I missed it, since it is a reusable mistake: I audited the writers and confirmed every one stamps local_time at write time, then treated the field as trustworthy. That answers "do our writers lie?" but not "can the reader be fed a value our writers never wrote?" — and the second question is the one that matters for a field newly promoted to first preference. A writer audit is necessary and not sufficient.
The suggested shape sounds right: consume local_time only where its pre-task: header provenance is established (the strict/trusted parse), and add the negative regression so the forged-body case is pinned rather than argued about.
I have no view to add on Blocker 2 beyond agreeing that lesson 15 asks for the real round trip; I have not run it either.
What I verified and still stands, for whoever picks this up: every current writer stamps local_time at write time — remote_gateway_bridge.py:4147, event_consumer.py:140, slack-bridge.py:1271, discord-bridge.py:4279, telegram-bridge.py:1024 all call local_time_value() with no argument; task-bridge.ts:157/:920 pass a locally generated timestamp, not a sender's. So the idea is sound; it is the provenance of the read that needs fixing, not the premise.
Independent of the AgentActivity stack. It is based on
main.Why
Live, on 2026-10-26, the app was quit with two messages queued at the broker. On relaunch at 08:43:34Z the bridge served and queued both, and 12 s later the new core's startup orphan check archived them unanswered:
timestamp:was 05:56:55Z, which is when the owner sent the message.local_time:was 19:43:34+11:00, which is when the bridge wrote it intotasks/. No result was ever written, so the bridge kept the attempts in flight and the room showed them as working indefinitely.What
classify.pyages a task fromlocal_time:first. Every task writer stamps it with its own write time: the remote gateway bridge, Discord, Slack, Telegram, the task bridge and event_consumer. Only the first token is parsed; the IANA zone after it is display only.timestamp:, the id's epoch-ms and the mtime remain the fallbacks, in that order.Evidence
python3 tests/task-orphan-check-classify.test.pyat the parentmain(bb2649e1b), with the new tests, and then at HEAD:The new cases:
timestamp:and a 12 s oldlocal_time:isfresh(age_from: local_time);orphan;local_time:falls back totimestamp:.ruff and
scripts/review-checks.share clean.Live: this is for the next DMG. The check is to quit the app, send a message, wait more than 5 min, and relaunch. The message should be answered, not listed in an orphan-recovery note.