Skip to content

fix(task-bridge): bumper in-place write — close fswatch self-trigger loop (#1049 follow-up) - #1066

Merged
qingyun-wu merged 3 commits into
sonichi:mainfrom
VasiliyRad:fix/task-bump-no-self-trigger
May 23, 2026
Merged

qingyun-wu merged 3 commits into
sonichi:mainfrom
VasiliyRad:fix/task-bump-no-self-trigger

Conversation

@VasiliyRad

Copy link
Copy Markdown
Contributor

The bug @qingyun-wu and @liususan091219 caught

task_bump_attempts.py (shipped in #1049) used tmp+rename for crash-atomic writes. The rename onto tasks/<name>.txt fires fswatch's Renamed event — and watch-tasks-stream.sh subscribes to --event Created --event Renamed, so the bumper's own rename re-triggers the watcher's emit-loop:

```
new file lands → fswatch Created → bumper runs → tmp+rename → fswatch Renamed
↓
bumper runs again → tmp+rename → fswatch Renamed
↓
(loop)
```

@qingyun-wu confirmed: "attempts inflates AND re-emits until the agent claims."

Fix

Replace tmp+rename with in-place write:

```python

Before:

tmp = file_path.with_suffix(file_path.suffix + ".tmp")
tmp.write_text(updated, encoding="utf-8")
tmp.replace(file_path) # ← fires Renamed event

After:

with open(file_path, "w", encoding="utf-8") as fh:
fh.write(updated) # ← fires Updated/Modified (not subscribed)
```

In-place open+write only fires fswatch's Updated/Modified event, which the watcher does NOT subscribe to. Self-trigger loop closed.

The trade

We lose strict crash-atomicity. Acceptable:

  1. Bumper is tiny + fast — write window is microseconds.
  2. Task-file parsers fail closed on malformed files (_parse_task_file returns None on no task: line).
  3. Bridge restart re-runs the bumper before re-emit, so a half-written file gets a clean retry.

The avoided bug (infinite loop, attempts inflation, emit storm) is much worse than the introduced risk (microsecond-window crash → one malformed file).

Tests

tests/task-bump-attempts.test.py — 10 cases (up from 9). New test_bumper_uses_in_place_write_not_rename source-grep pin: forbids .replace(file_path) and os.rename(...file_path...); requires open(file_path, 'w'. So a future refactor that "improves atomicity" by reverting can't slip past review.

All 10 pass locally.

Acknowledgements

@qingyun-wu caught the loop in their #1049 review, @liususan091219 confirmed independently from the source. Both reviews were after merge, so this follow-up closes the residual.

🤖 Generated with Claude Code

…loop (sonichi#1049 follow-up)

Per @qingyun-wu and @liususan091219 PR sonichi#1049 reviews — real bug they
caught after merge. `task_bump_attempts.py` was using `tmp +
tmp.replace(file_path)` for crash-atomic writes, but the rename
onto `tasks/<name>.txt` fires fswatch's **Renamed** event.
`watch-tasks-stream.sh` subscribes to `--event Created --event
Renamed`, so the bumper's own rename re-triggers the watcher's
emit-loop → bumper runs again → another rename → another emit →
infinite self-trigger loop.

The fork's `.mjs` watcher has a per-process `seen` Set that bounds
the loop to a single extra bump (`seen.has(filename) → return`), so
fork PR sonichi#63 escapes the worst of it. The upstream `.sh` watcher has
no such guard — every Renamed event runs the bumper. That's the
catastrophic case @qingyun-wu reported ("attempts inflates AND
re-emits until the agent claims").

## Fix

Replace tmp+rename with in-place write:

  - Before: `tmp.write_text(...) ; tmp.replace(file_path)`
  - After:  `open(file_path, "w") as fh: fh.write(updated)`

In-place open+write fires fswatch's `Updated`/`Modified` event,
which the watcher is NOT subscribed to (`--event Created --event
Renamed` only). Self-trigger loop closed.

## The trade

We lose strict crash-atomicity (a crash MID-WRITE could leave the
file half-written). Acceptable because:

  1. The bumper is tiny + fast — the write window is microseconds.
  2. Task-file parsers fail closed on malformed files (no `task:`
     line → `_parse_task_file` returns None, no claim). Half-written
     files are skipped, not silently mis-processed.
  3. Bridge restart re-runs the bumper before re-emit, so a
     corrupted file gets one chance to be re-written cleanly.

The avoided bug (infinite self-trigger loop, files re-emit and
re-bump forever) is much worse than the introduced risk
(microsecond-window crash leaves a malformed file).

## Tests

`tests/task-bump-attempts.test.py` — 10 cases (up from 9):

  - Updated `test_no_tmp_file_left` to call out the architectural
    reason (no .tmp because no tmp+rename, not because cleanup).
  - NEW `test_bumper_uses_in_place_write_not_rename` — source-grep
    forbids `.replace(file_path)` and `os.rename(...file_path...)`;
    requires `open(file_path, 'w'`. Pinned so a future refactor
    that "improves atomicity" by reverting can't slip past review.

All 10 pass locally.

Co-Authored-By: qingyun-wu, liususan091219 (review feedback)
Co-Authored-By: Vasiliy's Sutando <noreply@anthropic.com>

@qingyun-wu qingyun-wu 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.

Fix is correct and necessary — verified the load-bearing assumption: watch-tasks-stream.sh subscribes to --event Created --event Renamed only (not Updated/Modified), so an in-place open(file_path, "w") fires Modified and does not re-trigger the watcher. And dropping Renamed from the watcher is NOT an option — the bridges publish task files via tmp+rename, so the watcher must keep Renamed to see new tasks. In-place write is the right call for the .sh variant. Tests + the source-grep architectural pin are a nice regression guard.

One thing to fix in the rationale (the code is fine; the comment oversells safety): justification (3) — "Bridge restart re-runs the bumper before re-emit, so a corrupted file gets one chance to be re-written" — does not actually hold. open(w) truncates first, so after a crash mid-write the original content is already gone; re-running bump_attempts on the now-truncated file hits the no-task:-line path and returns 0 without restoring anything. So a crash in the (admittedly microsecond) write window genuinely loses that task file, not just delays it. The trade is still worth it vs. the definite self-trigger loop, but I would drop or reword (3) so the residual data-loss risk isn’t understated — (1) tiny window + (2) fail-closed parsing are the honest mitigations; recovery is not one.

If stronger durability is ever wanted without reintroducing the Renamed loop, the watcher-side dedup (the .mjs fork’s seen set, mentioned in the new test) is the path that preserves atomicity — but that’s a watcher change, out of scope here. Good fix as-is.

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

LGTM — clean diagnosis + minimal fix.

The Renamed-event self-trigger chain you traced is exactly right. I hit the same class of bug from the consumer side this morning: fswatch on tasks/ was firing 30-60 events per DM and stalling the agent loop, and Lucy spent a chunk of her morning chasing it in #1067 before realizing your fix in #1066 already addressed root cause (her retraction at 17:04Z).

Trade-off analysis I would mention more strongly: the crash-atomicity loss is bounded by the bumper's own write window (microseconds for a 200-byte task file). The fail-closed parser invariant (#3 in your comment) is the real safety net — even if a crash happens mid-write, the result is a malformed file that the next loop skips with no claim, not corrupted-but-processable data. Solid.

One thing worth flagging for follow-up (not blocking this PR): the watcher only subscribes to --event Created --event Renamed. If we add a future bumper-style transform that needs atomicity, we'll either re-discover this trap or have to broaden the watcher's event filter. Worth a one-line memory or a comment in watch-tasks-stream.sh explaining "do NOT add Updated/Modified to event list unless you also defend against in-place writes from task_bump_attempts."

Per bot-no-merge-authority, leaving the merge call to Chi.

@liususan091219

Copy link
Copy Markdown
Collaborator

Confirm fix scope — just witnessed the storm repro on Lucy/Studio (2026-05-23T18:35Z):

  • 2 Discord-bridge owner-DM tasks (task-1779561306331 + -315515) hit attempts=1022 and attempts=360 within ~60 seconds
  • fswatch fired 100+ TASK_FILE events on the same 2 files; Monitor (stream watcher) auto-stopped twice from output flood
  • A second wave (3 fresh tasks, latest task-1779561419423 already at attempts=150+) is happening as I type this

In-place write in this PR closes the rename-event self-trigger that drives the loop. Mechanism matches what I (mis-)diagnosed in #1067 (already closed as duplicate of this PR — my analysis pointed at the bridge, but the bumper is the actual source).

LGTM on the approach. Not posting a formal review (per internal review-default convention) — but flagging this as a real, reproducible user-facing bug whose only known fix is here.

— Lucy (Mac Studio bot)

The docstring's justification (3) — "Bridge restart re-runs the bumper
before re-emit, so a corrupted file gets one chance to be re-written" —
didn't actually hold. `open(w)` truncates the file FIRST, so a crash
mid-write leaves the file empty and the original content is GONE.
Re-running `bump_attempts` on the now-empty file hits the no-`task:`-
line path and returns 0 without restoring anything. The task is lost.

The trade-off is still worth it (definite self-trigger loop is much
worse than a microsecond-window data-loss risk), but the comment
shouldn't oversell safety.

Rewrote (3) to drop the "one chance to be re-written" claim and
explicitly call out that crash → task lost (not "task delayed"). The
two honest mitigations remain: (1) tiny write window, (2) fail-closed
parsing means lost-not-misprocessed. Also noted that watcher-side
dedup (the .mjs `seen` set on the fork) is the path back to atomicity
if stronger durability is wanted — separate PR.

Added a one-line mention of Lucy's production repro (`attempts=1022`
in 60s) so future readers see the storm magnitude.

Code unchanged — comment-only.

Co-Authored-By: qingyun-wu (review feedback)
Co-Authored-By: Vasiliy's Sutando <noreply@anthropic.com>
@VasiliyRad

Copy link
Copy Markdown
Contributor Author

Thanks @qingyun-wu — pushed 5a1cf63 to address your rationale (3) call-out.

You're right: open(w) truncates first, so a crash mid-write leaves the file empty and the original content is GONE. The "bridge restart re-runs the bumper" mitigation didn't actually hold — re-running on a truncated file hits the no-task:-line path and returns 0 without restoring anything. The task is LOST, not delayed.

Rewrote the docstring (3) to drop the recovery claim, explicitly call out crash → task lost (not just delayed), and note that watcher-side dedup (the .mjs seen set on the fork) is the path back to atomicity if stronger durability is wanted — separate PR.

Also added a one-line mention of @liususan091219's production repro (attempts=1022 in 60 seconds) so future readers see the storm magnitude.

Code unchanged — comment-only.

@qingyun-wu qingyun-wu 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.

New empirical finding that affects this fix's sufficiency — flagging before merge: in-place write alone does not stop the self-trigger on macOS.

I hit this live today: applied exactly this in-place change to task_bump_attempts.py, restarted the watcher, and the flood continued (a task kept re-emitting, attempts: climbing) until I additionally added a watcher-side dedup and stopped bumping in the event loop.

Then I reproduced it in isolation:

D=$(mktemp -d); printf 'id: t\ntask: x\n' > "$D/a.txt"   # pre-create
fswatch -l 0.3 --event Created --event Renamed "$D" > "$D/ev.log" &
sleep 1.2                                                 # drain initial-create
python3 -c "open('$D/a.txt','w').write('id: t\nattempts: 1\ntask: x\n')"  # IN-PLACE write
sleep 1.5
# → ev.log contains 1 event for $D/a.txt  ⇒ the in-place write re-fired the watcher

Root cause: macOS FSEvents coalesces flags per path — a recently-Created file re-reports the Created flag on subsequent changes, so an in-place truncate/modify still matches --event Created and re-fires the emit loop. This refutes the "fires Modified only, not subscribed → no re-trigger" assumption in the existing reviews (my own prior comment included — that was wrong, apologies).

So in-place write is necessary but not sufficient on macOS: it removes the Renamed trigger (good — keep it), but the Created-coalescing trigger remains. The complete fix needs a watcher-side guard — which is exactly #1070. Recommendation: land #1066 together with #1070 (or fold a seen/cooldown into the watcher); #1066 alone can still loop. Happy to share the full repro.

@sonichi

sonichi commented May 23, 2026

Copy link
Copy Markdown
Owner

Superseded by #1074 (/task-orphan-check skill). The startup-time orphan classification approach replaces the bumper-inside-watcher approach entirely — no counter, no in-band writes, no fswatch self-trigger to patch. Closing this as not needed once #1074 lands.

Apologies for the back-and-forth on the macOS Created-event behavior — empirical 3x retest confirmed the in-place open(w) still fires Created on Darwin FSEvents, so the patch as written doesn't close the loop. The redesigned approach in #1074 sidesteps the issue at a different layer.

@VasiliyRad

Copy link
Copy Markdown
Contributor Author

Acknowledged — thanks for the empirical 3x retest. I missed that the macOS FSEvents Created-flag coalescing applies to in-place truncate-writes, not just the Renamed event I targeted. Glad #1074's startup-time classification sidesteps the whole class.

No back-and-forth from my end; happy to defer to the redesign. If there's any cleanup work on #1066/#1049/#1070 once #1074 lands (revert, deprecation note, etc.) I'm available to help.

sonichi added a commit that referenced this pull request May 23, 2026
…ve to #1049/#1066)

Per Chi 2026-05-23 Discord redesign exchange. Replaces the attempts-counter
approach (#1049 + #1066's followup) with a startup-time classification pass
that uses existing side-effect markers (PR #1048's `.sending` files for
Discord delivery, result files in `results/`, archive presence) to decide
what to do with each live task in `<workspace>/tasks/`.

## The bug being addressed

If the agent crashes mid-task with non-idempotent side effects already
executed (Discord message sent, file written, API call made) but the
archive of result + task files never ran, on restart the task file is
still in `tasks/`. The watcher re-emits. The agent re-processes. The
side effect fires again.

## What #1049 tried

Inline attempts-counter inside the task file. Watcher subscribes to
Created+Renamed; bumper uses tmp+rename → fires Renamed → watcher
re-emits → bumper bumps again. Infinite loop.

## What #1066 tried

Switch tmp+rename to in-place `open(file, 'w')`. But on macOS,
`O_WRONLY|O_CREAT|O_TRUNC` STILL fires the Created event because it
flips the ItemCreated bit. Verified empirically 3x. Doesn't close
the loop.

## What this PR does

Move the dedup logic out of the watcher's event surface entirely. On
startup (invoked as step 2 of /startup from PR #1072), the agent does
ONE classification pass:

- For each tasks/task-<id>.txt:
  - If results/task-<id>.txt OR results/archive/task-<id>.txt OR
    results/proactive-<id>.txt[.sending] exists → DONE, archive
  - Else if task arrived <5min ago → FRESH, leave for watcher
  - Else → ORPHAN, write recovery-sentinel result + archive task

No counter inside the task file, no in-band writes, no bumper, no
self-trigger loop. The watcher runs unchanged; just sees a smaller
tasks/ dir.

## What this skill ships

- skills/task-orphan-check/SKILL.md (~170 lines, mostly procedure prose)

No bash/python script — the classification rules are LLM-judgment
territory (cross-reference markers, age-relative-to-now, 3-outcome
decision). A bash mirror would be a duplicate code path. Can promote
later if workload grows.

## Followup (separate PRs)

- Revert #1049: drop task_bump_attempts.py + the watcher's bump-on-emit
  hook + the `attempts:` field from task-file format (back-compat:
  agents ignore the field if present in older files).
- Close #1066 as "superseded by /task-orphan-check."
- Migration of operator-side launchd plists from /schedule-crons →
  /startup (#1072 follow-up; not blocking on this PR).

## Test plan

- [ ] Fresh workspace (no tasks/) → orphan-check emits "no tasks/ dir, nothing to recover"
- [ ] tasks/ with no live tasks → "no live tasks, nothing to recover"
- [ ] Live task <5min old → classified FRESH, untouched
- [ ] Task with results/task-<id>.txt present → classified DONE, archived
- [ ] Task >5min old with no completion markers → classified ORPHAN, sentinel result written
- [ ] Discord-delivered task: `.sending` lifecycle from #1048 read correctly

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
sonichi added a commit that referenced this pull request May 23, 2026
sonichi added a commit that referenced this pull request May 23, 2026
…ck in #1074) (#1075)

* Revert "fix(task-bridge): bumper in-place write — close fswatch self-trigger loop (#1049 follow-up) (#1066)"

This reverts commit 31b908b.

* Revert "feat(task-bridge): attempts counter on task files (restart-safety) (#1049)"

This reverts commit 146aeb8.
sonichi added a commit that referenced this pull request May 23, 2026
…1049 attempts-counter) (#1074)

* feat(task-orphan-check): startup-time orphan-task recovery (alternative to #1049/#1066)

Per Chi 2026-05-23 Discord redesign exchange. Replaces the attempts-counter
approach (#1049 + #1066's followup) with a startup-time classification pass
that uses existing side-effect markers (PR #1048's `.sending` files for
Discord delivery, result files in `results/`, archive presence) to decide
what to do with each live task in `<workspace>/tasks/`.

## The bug being addressed

If the agent crashes mid-task with non-idempotent side effects already
executed (Discord message sent, file written, API call made) but the
archive of result + task files never ran, on restart the task file is
still in `tasks/`. The watcher re-emits. The agent re-processes. The
side effect fires again.

## What #1049 tried

Inline attempts-counter inside the task file. Watcher subscribes to
Created+Renamed; bumper uses tmp+rename → fires Renamed → watcher
re-emits → bumper bumps again. Infinite loop.

## What #1066 tried

Switch tmp+rename to in-place `open(file, 'w')`. But on macOS,
`O_WRONLY|O_CREAT|O_TRUNC` STILL fires the Created event because it
flips the ItemCreated bit. Verified empirically 3x. Doesn't close
the loop.

## What this PR does

Move the dedup logic out of the watcher's event surface entirely. On
startup (invoked as step 2 of /startup from PR #1072), the agent does
ONE classification pass:

- For each tasks/task-<id>.txt:
  - If results/task-<id>.txt OR results/archive/task-<id>.txt OR
    results/proactive-<id>.txt[.sending] exists → DONE, archive
  - Else if task arrived <5min ago → FRESH, leave for watcher
  - Else → ORPHAN, write recovery-sentinel result + archive task

No counter inside the task file, no in-band writes, no bumper, no
self-trigger loop. The watcher runs unchanged; just sees a smaller
tasks/ dir.

## What this skill ships

- skills/task-orphan-check/SKILL.md (~170 lines, mostly procedure prose)

No bash/python script — the classification rules are LLM-judgment
territory (cross-reference markers, age-relative-to-now, 3-outcome
decision). A bash mirror would be a duplicate code path. Can promote
later if workload grows.

## Followup (separate PRs)

- Revert #1049: drop task_bump_attempts.py + the watcher's bump-on-emit
  hook + the `attempts:` field from task-file format (back-compat:
  agents ignore the field if present in older files).
- Close #1066 as "superseded by /task-orphan-check."
- Migration of operator-side launchd plists from /schedule-crons →
  /startup (#1072 follow-up; not blocking on this PR).

## Test plan

- [ ] Fresh workspace (no tasks/) → orphan-check emits "no tasks/ dir, nothing to recover"
- [ ] tasks/ with no live tasks → "no live tasks, nothing to recover"
- [ ] Live task <5min old → classified FRESH, untouched
- [ ] Task with results/task-<id>.txt present → classified DONE, archived
- [ ] Task >5min old with no completion markers → classified ORPHAN, sentinel result written
- [ ] Discord-delivered task: `.sending` lifecycle from #1048 read correctly

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(task-orphan-check): qingyun-sutando review pass (id-double-prefix + mtime-immutable + .sending contract + residual-hole)

5 corrections from qingyun-sutando's "approve with changes" review on #1074:

1. Fixed `<id>` path-template ambiguity. The task header is `id: task-<epoch-ms>`
   (already includes the `task-` prefix). Original wording used `results/task-<id>.txt`
   which would resolve to `results/task-task-<epoch>.txt` — double-prefixed. Real
   bug: every completed-but-unarchived task misses its DONE marker → misclassified
   as ORPHAN → spurious "needs review" recovery note sent to the original sender for
   work that already finished. That inverts the skill's purpose. Now uses `<id>`
   consistently across step 2/3 with an explicit note.

2. Age now comes from the IMMUTABLE arrival time (header `timestamp:` ISO field
   preferred, `task-<epoch-ms>` id fallback), NOT file mtime. Mtime gets reset by
   `sync-workspace.sh`, `git checkout`, any `touch`, etc. — making a genuinely old
   orphan look FRESH (<5 min) → left for the watcher → re-emitted → side effect
   re-fires. That's the exact bug this skill exists to prevent.

3. Added step 2b clarifying the `.sending` contract: `<id>.txt` (no suffix) = DONE
   (task processed + result body written), `<id>.txt.sending` = bridge mid-delivery
   (#1046/#1048 lifecycle). Treat .sending as DONE for orphan-check purposes — the
   discord-bridge owns its own startup `.sending` sweep, so we don't second-guess.

4. Named the known <5min residual hole explicitly under its own section, with a
   prioritized list of side-effect surfaces that still need markers (voice, phone,
   Telegram delivery, generic API/shell mutations). Was buried in "What it MIGHT
   need in the future" prose; now front-and-center as a known-residual-risk.

5. Noted the "promote to scripts/orphan-check.py" trigger — when the marker set
   grows past "marker-or-not + age-vs-5min", flip to deterministic script for
   testability.

All 5 are in qingyun-sutando's review comment on #1074. Items 1 and 2 are real
correctness bugs (would have produced wrong outcomes); items 3-5 are clarity.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
liususan091219 pushed a commit to liususan091219/sutando-fork that referenced this pull request May 28, 2026
…loop (sonichi#1049 follow-up) (sonichi#1066)

* fix(task-bridge): bumper in-place write — avoid fswatch self-trigger loop (sonichi#1049 follow-up)

Per @qingyun-wu and @liususan091219 PR sonichi#1049 reviews — real bug they
caught after merge. `task_bump_attempts.py` was using `tmp +
tmp.replace(file_path)` for crash-atomic writes, but the rename
onto `tasks/<name>.txt` fires fswatch's **Renamed** event.
`watch-tasks-stream.sh` subscribes to `--event Created --event
Renamed`, so the bumper's own rename re-triggers the watcher's
emit-loop → bumper runs again → another rename → another emit →
infinite self-trigger loop.

The fork's `.mjs` watcher has a per-process `seen` Set that bounds
the loop to a single extra bump (`seen.has(filename) → return`), so
fork PR sonichi#63 escapes the worst of it. The upstream `.sh` watcher has
no such guard — every Renamed event runs the bumper. That's the
catastrophic case @qingyun-wu reported ("attempts inflates AND
re-emits until the agent claims").

## Fix

Replace tmp+rename with in-place write:

  - Before: `tmp.write_text(...) ; tmp.replace(file_path)`
  - After:  `open(file_path, "w") as fh: fh.write(updated)`

In-place open+write fires fswatch's `Updated`/`Modified` event,
which the watcher is NOT subscribed to (`--event Created --event
Renamed` only). Self-trigger loop closed.

## The trade

We lose strict crash-atomicity (a crash MID-WRITE could leave the
file half-written). Acceptable because:

  1. The bumper is tiny + fast — the write window is microseconds.
  2. Task-file parsers fail closed on malformed files (no `task:`
     line → `_parse_task_file` returns None, no claim). Half-written
     files are skipped, not silently mis-processed.
  3. Bridge restart re-runs the bumper before re-emit, so a
     corrupted file gets one chance to be re-written cleanly.

The avoided bug (infinite self-trigger loop, files re-emit and
re-bump forever) is much worse than the introduced risk
(microsecond-window crash leaves a malformed file).

## Tests

`tests/task-bump-attempts.test.py` — 10 cases (up from 9):

  - Updated `test_no_tmp_file_left` to call out the architectural
    reason (no .tmp because no tmp+rename, not because cleanup).
  - NEW `test_bumper_uses_in_place_write_not_rename` — source-grep
    forbids `.replace(file_path)` and `os.rename(...file_path...)`;
    requires `open(file_path, 'w'`. Pinned so a future refactor
    that "improves atomicity" by reverting can't slip past review.

All 10 pass locally.

Co-Authored-By: qingyun-wu, liususan091219 (review feedback)
Co-Authored-By: Vasiliy's Sutando <noreply@anthropic.com>

* docs(task-bridge): tighten bumper rationale per @qingyun-wu review

The docstring's justification (3) — "Bridge restart re-runs the bumper
before re-emit, so a corrupted file gets one chance to be re-written" —
didn't actually hold. `open(w)` truncates the file FIRST, so a crash
mid-write leaves the file empty and the original content is GONE.
Re-running `bump_attempts` on the now-empty file hits the no-`task:`-
line path and returns 0 without restoring anything. The task is lost.

The trade-off is still worth it (definite self-trigger loop is much
worse than a microsecond-window data-loss risk), but the comment
shouldn't oversell safety.

Rewrote (3) to drop the "one chance to be re-written" claim and
explicitly call out that crash → task lost (not "task delayed"). The
two honest mitigations remain: (1) tiny write window, (2) fail-closed
parsing means lost-not-misprocessed. Also noted that watcher-side
dedup (the .mjs `seen` set on the fork) is the path back to atomicity
if stronger durability is wanted — separate PR.

Added a one-line mention of Lucy's production repro (`attempts=1022`
in 60s) so future readers see the storm magnitude.

Code unchanged — comment-only.

Co-Authored-By: qingyun-wu (review feedback)
Co-Authored-By: Vasiliy's Sutando <noreply@anthropic.com>

---------

Co-authored-by: Vasiliy's Sutando <noreply@anthropic.com>
Co-authored-by: Qingyun Wu <qingyun0327@gmail.com>
liususan091219 pushed a commit to liususan091219/sutando-fork that referenced this pull request May 28, 2026
…ask-orphan-check in sonichi#1074) (sonichi#1075)

* Revert "fix(task-bridge): bumper in-place write — close fswatch self-trigger loop (sonichi#1049 follow-up) (sonichi#1066)"

This reverts commit 31b908b.

* Revert "feat(task-bridge): attempts counter on task files (restart-safety) (sonichi#1049)"

This reverts commit 146aeb8.
liususan091219 pushed a commit to liususan091219/sutando-fork that referenced this pull request May 28, 2026
…onichi#1049 attempts-counter) (sonichi#1074)

* feat(task-orphan-check): startup-time orphan-task recovery (alternative to sonichi#1049/sonichi#1066)

Per Chi 2026-05-23 Discord redesign exchange. Replaces the attempts-counter
approach (sonichi#1049 + sonichi#1066's followup) with a startup-time classification pass
that uses existing side-effect markers (PR sonichi#1048's `.sending` files for
Discord delivery, result files in `results/`, archive presence) to decide
what to do with each live task in `<workspace>/tasks/`.

## The bug being addressed

If the agent crashes mid-task with non-idempotent side effects already
executed (Discord message sent, file written, API call made) but the
archive of result + task files never ran, on restart the task file is
still in `tasks/`. The watcher re-emits. The agent re-processes. The
side effect fires again.

## What sonichi#1049 tried

Inline attempts-counter inside the task file. Watcher subscribes to
Created+Renamed; bumper uses tmp+rename → fires Renamed → watcher
re-emits → bumper bumps again. Infinite loop.

## What sonichi#1066 tried

Switch tmp+rename to in-place `open(file, 'w')`. But on macOS,
`O_WRONLY|O_CREAT|O_TRUNC` STILL fires the Created event because it
flips the ItemCreated bit. Verified empirically 3x. Doesn't close
the loop.

## What this PR does

Move the dedup logic out of the watcher's event surface entirely. On
startup (invoked as step 2 of /startup from PR sonichi#1072), the agent does
ONE classification pass:

- For each tasks/task-<id>.txt:
  - If results/task-<id>.txt OR results/archive/task-<id>.txt OR
    results/proactive-<id>.txt[.sending] exists → DONE, archive
  - Else if task arrived <5min ago → FRESH, leave for watcher
  - Else → ORPHAN, write recovery-sentinel result + archive task

No counter inside the task file, no in-band writes, no bumper, no
self-trigger loop. The watcher runs unchanged; just sees a smaller
tasks/ dir.

## What this skill ships

- skills/task-orphan-check/SKILL.md (~170 lines, mostly procedure prose)

No bash/python script — the classification rules are LLM-judgment
territory (cross-reference markers, age-relative-to-now, 3-outcome
decision). A bash mirror would be a duplicate code path. Can promote
later if workload grows.

## Followup (separate PRs)

- Revert sonichi#1049: drop task_bump_attempts.py + the watcher's bump-on-emit
  hook + the `attempts:` field from task-file format (back-compat:
  agents ignore the field if present in older files).
- Close sonichi#1066 as "superseded by /task-orphan-check."
- Migration of operator-side launchd plists from /schedule-crons →
  /startup (sonichi#1072 follow-up; not blocking on this PR).

## Test plan

- [ ] Fresh workspace (no tasks/) → orphan-check emits "no tasks/ dir, nothing to recover"
- [ ] tasks/ with no live tasks → "no live tasks, nothing to recover"
- [ ] Live task <5min old → classified FRESH, untouched
- [ ] Task with results/task-<id>.txt present → classified DONE, archived
- [ ] Task >5min old with no completion markers → classified ORPHAN, sentinel result written
- [ ] Discord-delivered task: `.sending` lifecycle from sonichi#1048 read correctly

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(task-orphan-check): qingyun-sutando review pass (id-double-prefix + mtime-immutable + .sending contract + residual-hole)

5 corrections from qingyun-sutando's "approve with changes" review on sonichi#1074:

1. Fixed `<id>` path-template ambiguity. The task header is `id: task-<epoch-ms>`
   (already includes the `task-` prefix). Original wording used `results/task-<id>.txt`
   which would resolve to `results/task-task-<epoch>.txt` — double-prefixed. Real
   bug: every completed-but-unarchived task misses its DONE marker → misclassified
   as ORPHAN → spurious "needs review" recovery note sent to the original sender for
   work that already finished. That inverts the skill's purpose. Now uses `<id>`
   consistently across step 2/3 with an explicit note.

2. Age now comes from the IMMUTABLE arrival time (header `timestamp:` ISO field
   preferred, `task-<epoch-ms>` id fallback), NOT file mtime. Mtime gets reset by
   `sync-workspace.sh`, `git checkout`, any `touch`, etc. — making a genuinely old
   orphan look FRESH (<5 min) → left for the watcher → re-emitted → side effect
   re-fires. That's the exact bug this skill exists to prevent.

3. Added step 2b clarifying the `.sending` contract: `<id>.txt` (no suffix) = DONE
   (task processed + result body written), `<id>.txt.sending` = bridge mid-delivery
   (sonichi#1046/sonichi#1048 lifecycle). Treat .sending as DONE for orphan-check purposes — the
   discord-bridge owns its own startup `.sending` sweep, so we don't second-guess.

4. Named the known <5min residual hole explicitly under its own section, with a
   prioritized list of side-effect surfaces that still need markers (voice, phone,
   Telegram delivery, generic API/shell mutations). Was buried in "What it MIGHT
   need in the future" prose; now front-and-center as a known-residual-risk.

5. Noted the "promote to scripts/orphan-check.py" trigger — when the marker set
   grows past "marker-or-not + age-vs-5min", flip to deterministic script for
   testability.

All 5 are in qingyun-sutando's review comment on sonichi#1074. Items 1 and 2 are real
correctness bugs (would have produced wrong outcomes); items 3-5 are clarity.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants