Skip to content

fix(task-orphan-check): age from local_time only when it is a real header, never a body line [AgentActivity] - #6028

Merged
marklysze merged 1 commit into
sonichi:mainfrom
marklysze:fix/orphan-check-local-time-header-only
Oct 30, 2026
Merged

marklysze merged 1 commit into
sonichi:mainfrom
marklysze:fix/orphan-check-local-time-header-only

Conversation

@marklysze

Copy link
Copy Markdown
Collaborator

Follow-up to #5929, which is merged. Fixes the post-merge blocker from @keweichen's review (independently confirmed by @liususan091219), plus @sonichi's wording nits. Based on main (d55138c).

The bug on main

classify_task parses with parse_task_headers_lenient, and #5929 made local_time the first age source. That parser's own contract (src/local_task_protocol.py) says that when a writer legitimately lacks a key, a body line can supply it. So for a task from a writer that never stamps local_time: (older writers, other task shapes), a local_time: line in the user's body becomes the age. A fresh task then reads as an orphan and is archived unanswered at startup.

The fix

Wording (sonichi's nits):

  • The classifier docstring and SKILL.md no longer say "every writer stamps its own write time". They now say the local writer stamps it at intake: task-bridge.ts builds it from the timestamp it has just taken, and build_requeued_task keeps the original's, so a requeue does not make a task look fresh.
  • status_ack_policy.intake_at, SKILL.md and the docstring now say that status-ack's deadline deliberately stays on the sender's timestamp:, because it measures how long the sender has waited.

Evidence

python3 tests/task-orphan-check-classify.test.py, with the new tests, at the parent main (d55138c):

FAIL: test_a_local_time_line_in_a_legacy_body_is_not_the_age (__main__.TestInboxAge.test_a_local_time_line_in_a_legacy_body_is_not_the_age)
AssertionError: Tuples differ: ('orphan', 'local_time', 216740400) != ('fresh', 'timestamp', 12)
Ran 68 tests in 0.281s
FAILED (failures=1)

At HEAD:

Ran 68 tests in 0.285s

OK

New tests:

  • test_a_local_time_line_in_a_legacy_body_is_not_the_age (keweichen's repro shape): a 12 s old task whose writer stamps no local_time:, with local_time: 2019-10-30T00:00:00+00:00 UTC in the body. It is fresh, aged from timestamp.
  • test_a_real_header_wins_over_a_body_copy: the header above task: is used, not a body copy.

tests/status-ack-policy.test.py passes. ruff, scripts/review-checks.sh and gen-src-map --check are clean.

Live

The post-restart witness keweichen asked for (named lane, app quit, a message held at the broker for more than 5 min, relaunch, the message answered and not archived) is being scheduled on the rig. It will be pasted here when run. The fix is read-side only, in the startup classifier.

…ader

classify_task read local_time from the lenient parse, so for a writer that
never stamps it a local_time: line in the user's body became the age and a
fresh task was archived unanswered at startup. local_time now comes from the
strict pre-task: headers only; otherwise the age falls back to timestamp as
before. Also corrects the wording on who stamps local_time and notes that
status-ack's deadline stays on the sender's timestamp on purpose.

@sonichi sonichi left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Approving at 1eae439. This fixes the hole I missed in #5929: classify_task used the lenient parse, so a body local_time: line could become the age for any task whose writer never stamps the header. Now local_time comes only from the strict pre-task: headers (ltp.parse_task_headers), and task_queued_at takes it as a separate argument, so the lenient headers can't feed it. Both new tests cover the case: a body line in a legacy task, and a real header beating a body copy. tests/task-orphan-check-classify.test.py passes locally (68/68). CI was still running when I reviewed. The wording now matches the code, including the status_ack_policy.intake_at split.

(agent-assisted, Blue for Chi's seat)

@keweichen keweichen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Changes requested at 1eae439d — the code fix is correct, but this startup-path PR is not yet approvable.

The parser-provenance blocker from #5929 is fixed. I verified the exact head (68/68 classifier tests, 81/81 status-ack tests, 11/11 local-time writer tests, review scanner and src-map checks clean). The parent negative control at d55138c fails exactly the forged-body case: ('orphan', 'local_time', 216740400) instead of ('fresh', 'timestamp', 12). At HEAD a body-only local_time: falls back to the immutable timestamp, while a real pre-task: header still wins. I found no further code blocker.

Remaining blocker: attach the completed real post-restart witness described in the PR body (named lane; app quit; message held by the broker for >5 minutes; relaunch; that message is answered and is not archived/reported as recovered). The body currently says this is only being scheduled, and no witness is attached. Because this classifier runs on /startup and can archive owner tasks, REVIEW.md lesson 15 makes that end-to-end restarted-service round trip a review-time gate. The worst case without it is a fresh broker-held owner message being archived unanswered despite the unit proof.

Please re-request review after the witness is posted and CI is green.

@github-actions

Copy link
Copy Markdown
Contributor

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 94%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

  • skills/task-orphan-check/scripts/classify.py (100%)

Summary

  • Total: 3 lines
  • Missing: 0 lines
  • Coverage: 100%

@marklysze

Copy link
Copy Markdown
Collaborator Author

Live post-restart witness for #6028 (run by Mark's coordinator agent on Mark's Mac, local k3d rig)

Engine: test/orphan-fix-witness at c8a3a2ef4 = release 81d0c7397 + exactly this PR's diff (classify.py byte-identical to 1eae439). It contains the release sha, so the app updater left it in place across the relaunch.

  • HEAD before quit / after relaunch: c8a3a2ef4d813defbd46b64f5a464dec07ea209d / c8a3a2ef4d813defbd46b64f5a464dec07ea209d
  • Quit: real app quit (osascript quit); no bridge or keepalive process and no sutando-core tmux session afterwards. No sutando-ctl was needed.
  • T1 (mention sent while the agent was offline, named lane ag2space_dlocal): 2026-10-30T03:21:30Z
  • T2 (app relaunched): 2026-10-30T03:28:36Z (7 min 6 s held at the broker)

a. Engine after relaunch: c8a3a2ef4… (unchanged).
b. Task: tasks/archive/task-ag2space_dlocal~task-c73292776810e396e1.txt (named-lane id)
c. Headers (above task:): timestamp: 2026-10-30T03:21:30Z, local_time: 2026-10-30T14:28:50+11:00 Australia/Sydney (= 03:28:50Z, intake on relaunch).
d. Answered: one PONG-6028 from the agent in the room at 03:29:56Z. Bridge log:

  • 03:28:50Z served task-c73292776810e396e1 / queued task-ag2space_dlocal~task-c73292776810e396e1
  • 03:30:05Z result …: settled without a post (terminal operation succeeded)
  • 03:30:05Z archived … (marker REPLIED, lease closed, not sent)
    e. Not an orphan: archived only after REPLIED; 0 orphan proactive DMs name it.
    f. Classifier replay on that exact file with --now = T2+30 s: verdict: fresh, age_from: local_time, age_s: 16 (from timestamp it would be ~7.5 min, an orphan).

Restored afterwards: engine back at 81d0c7397, tree clean.

@keweichen keweichen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approved at 1eae439d — ready to merge.

The sole blocker from my prior review is resolved by the posted live witness. It exercised the stated restarted path: real app quit, a named-lane mention held at the broker for 426 seconds, relaunch, one answer, archival only after the REPLIED marker, and no orphan recovery message. The witness timestamps independently reconcile to a 03:28:50Z local intake and a 16-second classifier age; without that strict local_time: header, the sender timestamp would be old enough to trigger the failure mode.

I also verified the public witness commit c8a3a2ef4 uses byte-identical blobs to this PR head for all four changed non-changelog files, including the runtime classifier. Combined with the exact-head 68/68 classifier suite, the parent negative control, 81/81 status-ack tests, 11/11 writer tests, clean review scan, and fully green CI + CLA, this mitigates the worst case: archiving a fresh broker-held owner task unanswered. No remaining blocker or nit.

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