Skip to content

fix(task-bridge): unify timeout archive path → tasks/archive/<YYYY-MM>/ - #591

Merged
sonichi merged 1 commit into
mainfrom
fix/timeout-archive-path
May 5, 2026
Merged

fix(task-bridge): unify timeout archive path → tasks/archive/<YYYY-MM>/#591
sonichi merged 1 commit into
mainfrom
fix/timeout-archive-path

Conversation

@sonichi

@sonichi sonichi commented May 4, 2026

Copy link
Copy Markdown
Owner

Summary

Mini flagged on the #589 cold review: the timeout path moved task files to tasks/processed/ while every other "task is done" path uses archiveFile() → tasks/archive/<YYYY-MM>/. Two destinations means the learn-collector / summarizer would have to scan both.

Replaced the inline mkdir + renameSync with a single archiveFile() call. Same shape as the result-delivery + deduped + voice-offline paths.

_isVoiceTask() already checks both processed/ AND archive/ when looking up source channel, so no breakage for tasks already in processed/ from before this PR.

Test plan

  • Submit a task without writing a result; wait timeout_minutes (default 10m); confirm task file lands in tasks/archive/<YYYY-MM>/<id>.txt instead of tasks/processed/<id>.txt.
  • Confirm UI clears the task at timeout (no stuck spinner).
  • Existing tasks in tasks/processed/ (legacy) — _isVoiceTask still resolves them; no functional regression.

🤖 Generated with Claude Code

@sonichi
fix(task-bridge): unify timeout archive path → tasks/archive/<YYYY-MM>/
f9a4177 
Mini flagged on the #589 cold review: the timeout path moved task
files to tasks/processed/ while every other "task is done" path uses
archiveFile() → tasks/archive/<YYYY-MM>/. Two destinations means
the learn-collector / summarizer would have to scan both.

Replaced the inline mkdir+renameSync with a single archiveFile() call.
Same shape as the result-delivery + deduped + voice-offline paths.

_isVoiceTask() already checks both processed/ AND archive/ when
looking up source channel, so no breakage for tasks already in
processed/ from before this PR.
sonichi
sonichi commented May 5, 2026
View reviewed changes

@sonichi sonichi left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

LGTM — addresses my #589 flag exactly.

Change: timeout-cleanup branch in startResultWatcher (line 456-468 before, 456-466 after) now goes through the same archiveFile() helper as every other "task is done" path, instead of the bespoke mkdir + renameSync → tasks/processed/. Single destination = single scan footprint for learn-collector, summarizer, and any future archive consumer.

Verified:

  • archiveFile() (line 47–57) writes to tasks/archive/<YYYY-MM>/. ✓
  • Lost behavior: previous code logged to console.error on archive failure. New code (via archiveFile) silently falls back to unlinkSync. This matches every other archive call site so the loss-of-log is the consistent shape, not a regression.
  • The lone tasks/processed/ reference in the file (line 161) is just status prose — not a path. ✓

One note (non-blocking): the PR description references _isVoiceTask checking both processed/ AND archive/. I can't find that function in src/ or on this branch. I think the description is from an earlier draft. Doesn't matter for correctness — grep -rn "tasks/processed" src/ returns no readers, and the tasks/processed/ directory doesn't exist on disk. So no backward-compat concern: there's nothing for which to be backward-compatible.

Gate (sanity check):

  • Import strategy: no new imports ✓
  • Defaults: no behavior change for legitimate flows; only affects timeout-cleanup destination ✓
  • Bind-address: N/A ✓

Approve. Recommend merging via gh pr merge --squash (no --delete-branch per repo norm).

@sonichi
sonichi merged commit a306324 into main May 5, 2026
1 check passed
@sonichi
sonichi deleted the fix/timeout-archive-path branch May 5, 2026 16:58
This was referenced May 7, 2026
Closed
Merged
sonichi added a commit that referenced this pull request May 7, 2026
@sonichi Chi @claude
fix(task-bridge): _isVoiceTask scans month-partitioned archive subdirs (
e0f9901 
#622)

Concrete bug: archiveFile() writes archived task files into
tasks/archive/YYYY-MM/<taskId>.txt (PR #591 layout), but _isVoiceTask()
only checked the legacy flat path tasks/archive/<taskId>.txt. After
archive, voice classification silently fell back to false — the
offline-forwarding gate at task-bridge.ts:535 then misclassified
archived voice tasks as non-voice and suppressed DM-fallback delivery.

Fix: glob the month subdirs under tasks/archive/ (regex-gated to
YYYY-MM names so stray subdirs like `done/` are skipped). Legacy
flat-archive path retained for any task archived pre-#591.

Tests: new tests/task-bridge-isvoice.test.ts — 8 cases covering
live / processed / legacy-flat / month-partitioned positive paths,
non-voice negative path, stray-subdir negative control, missing-file
return-false, and multi-month-subdir traversal. All pass.

Out of scope (per VasiliyRad's review): the broader task-classification
decision-boundary refactor stays in Path D Stage 2. This PR is just
the path-drift fix.

Co-authored-by: Chi <wangchi@Chis-Mac-mini.hsd1.wa.comcast.net>
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
@sonichi
fix(task-bridge): unify timeout archive path → tasks/archive/<YYYY-MM…
d6ccd12 
…>/ (sonichi#591)

Mini flagged on the sonichi#589 cold review: the timeout path moved task
files to tasks/processed/ while every other "task is done" path uses
archiveFile() → tasks/archive/<YYYY-MM>/. Two destinations means
the learn-collector / summarizer would have to scan both.

Replaced the inline mkdir+renameSync with a single archiveFile() call.
Same shape as the result-delivery + deduped + voice-offline paths.

_isVoiceTask() already checks both processed/ AND archive/ when
looking up source channel, so no breakage for tasks already in
processed/ from before this PR.
liususan091219 pushed a commit to liususan091219/sutando-fork that referenced this pull request May 28, 2026
@sonichi Chi @claude
fix(task-bridge): _isVoiceTask scans month-partitioned archive subdirs (
67af2c2 
sonichi#622)

Concrete bug: archiveFile() writes archived task files into
tasks/archive/YYYY-MM/<taskId>.txt (PR sonichi#591 layout), but _isVoiceTask()
only checked the legacy flat path tasks/archive/<taskId>.txt. After
archive, voice classification silently fell back to false — the
offline-forwarding gate at task-bridge.ts:535 then misclassified
archived voice tasks as non-voice and suppressed DM-fallback delivery.

Fix: glob the month subdirs under tasks/archive/ (regex-gated to
YYYY-MM names so stray subdirs like `done/` are skipped). Legacy
flat-archive path retained for any task archived pre-sonichi#591.

Tests: new tests/task-bridge-isvoice.test.ts — 8 cases covering
live / processed / legacy-flat / month-partitioned positive paths,
non-voice negative path, stray-subdir negative control, missing-file
return-false, and multi-month-subdir traversal. All pass.

Out of scope (per VasiliyRad's review): the broader task-classification
decision-boundary refactor stays in Path D Stage 2. This PR is just
the path-drift fix.

Co-authored-by: Chi <wangchi@Chis-Mac-mini.hsd1.wa.comcast.net>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@qingyun-wu qingyun-wu mentioned this pull request Jun 20, 2026
Merged
sonichi added a commit that referenced this pull request Aug 2, 2026
@sonichi
fix(morning-briefing): scan the month-partitioned archive, not just t…
e42cc19 
…he legacy flat one

Review finding from qingyun-wu on b30a763, confirmed: the archive is
month-partitioned (tasks/archive/YYYY-MM/<id>.txt, PR #591) and the flat form
is legacy. My non-recursive glob over tasks/archive/ scanned only the legacy
shape, so a DM archived by the bridge in the normal path was still missed —
preserving the same false-clean the PR exists to remove, just smaller.

Measured on the live workspace:

    flat (what the previous head scanned)   280 files
    month-partitioned (missed)              178 files
    owner DMs in an 8h window: 1 found, 2 MISSED

So the briefing would have said "Overnight: 1 Discord message" where the truth
was 3. Before/after on the live workspace, same moment:

    PREVIOUS head   1 message
    THIS commit     3 messages

My own "verified 269 discord task files, 100% parseable" in the original commit
message was measured with that same flat glob — I measured the legacy subset
and drew a conclusion about the whole archive.

Scans one level deep only, matching discord-bridge.py's own
`archive.glob(f"*/{task_id}.txt")` idiom rather than rglob, which would walk
unbounded depth.

Tests: 3 added — month-partitioned archive found; flat and month merged in time
order (so the legacy shape is not dropped); and the scan bounded to one level.
Exactly 2 of the 16 fail at the previous head (the two asserting month
scanning); the depth guard passes at both heads by design, since it constrains
the NEW behaviour. 16/16 here, and all seven morning-briefing-touching suites
pass.

Stand: Echo Act IV Mini
john-the-dev added a commit that referenced this pull request Aug 3, 2026
@sonichi @john-the-dev
fix(morning-briefing): the "last 8 hours" of Discord DMs was really a…
fe8197c 
… 200-LINE window (#2508)

* fix(morning-briefing): the "last 8 hours" of Discord DMs was a 200-LINE window

get_overnight_discord() promised an 8-hour window in its docstring, computed
`cutoff = time.time() - 8 * 3600`, and then never applied it. The effective
window was `splitlines()[-200:]` on logs/discord-bridge.log — a line count that
silently shrinks as the bridge gets chattier.

Measured on the live log 2026-08-02: 6,754 lines, 9 matching DM lines at indices
596-6177, window starting at 6,554. Zero of the nine were inside it, so the
briefing reported NO overnight messages on a day that had them. The failure
direction is false-clean, which is the worst one for a daily briefing.

This is not a one-line fix to apply the dead variable, because the log cannot
answer the question: its `[msg] #DM` lines carry no timestamp (10 of 6,754 lines
in the whole file have an ISO stamp, none of them message lines). There is
nothing for a cutoff to compare against.

The bridge already writes a properly timestamped record of every DM it handles:
tasks/task-<id>.txt, archived to tasks/archive/, each with an ISO `timestamp:`,
`source:`, `channel_name:` and `access_tier:`. Reading those makes the promised
8-hour filter implementable with no new instrumentation. Verified on the live
workspace: 269 discord task files, 3-day span, 100% parseable.

Owner DMs are `source: discord` + `channel_name: DM` + `access_tier: owner`.
The tier check replaces the old sender-name exclusion — peer bots post to shared
channels as `team`, so they cannot reach this list.

Before / after on the live workspace, same moment:

    BEFORE  0 messages  -> briefing omits the overnight line
    AFTER   1 message   -> "Overnight: 1 Discord message."

The one it now finds is a real owner DM at 04:16Z that the shipped code missed.

Tests: 13 new. The fixture for the central case writes BOTH a chatty log with a
real DM line pushed past the 200-line window AND the task file, so the shipped
function returns [] on it where the new one returns the message — the control is
behavioural, not a signature mismatch. Also covers the >8h exclusion (the whole
point, and something a line window cannot express), archive+live scanning,
channel-vs-DM, tier, source, the 5-cap, unparseable timestamps, truncation, an
unreadable entry, and a header-only task file. 15/15 morning-briefing-touching
test files pass.

Not changed, deliberately: synthesize() speaks only len(discord_msgs), so the
per-message text this gatherer returns is still discarded. Making the briefing
say WHAT arrived is an editorial change to a spoken deliverable and belongs in
its own PR.

Stand: Echo Act IV Mini

* fix(morning-briefing): scan the month-partitioned archive, not just the legacy flat one

Review finding from qingyun-wu on b30a763, confirmed: the archive is
month-partitioned (tasks/archive/YYYY-MM/<id>.txt, PR #591) and the flat form
is legacy. My non-recursive glob over tasks/archive/ scanned only the legacy
shape, so a DM archived by the bridge in the normal path was still missed —
preserving the same false-clean the PR exists to remove, just smaller.

Measured on the live workspace:

    flat (what the previous head scanned)   280 files
    month-partitioned (missed)              178 files
    owner DMs in an 8h window: 1 found, 2 MISSED

So the briefing would have said "Overnight: 1 Discord message" where the truth
was 3. Before/after on the live workspace, same moment:

    PREVIOUS head   1 message
    THIS commit     3 messages

My own "verified 269 discord task files, 100% parseable" in the original commit
message was measured with that same flat glob — I measured the legacy subset
and drew a conclusion about the whole archive.

Scans one level deep only, matching discord-bridge.py's own
`archive.glob(f"*/{task_id}.txt")` idiom rather than rglob, which would walk
unbounded depth.

Tests: 3 added — month-partitioned archive found; flat and month merged in time
order (so the legacy shape is not dropped); and the scan bounded to one level.
Exactly 2 of the 16 fail at the previous head (the two asserting month
scanning); the depth guard passes at both heads by design, since it constrains
the NEW behaviour. 16/16 here, and all seven morning-briefing-touching suites
pass.

Stand: Echo Act IV Mini

* fix(morning-briefing): bound the overnight window on BOTH sides

@john-the-dev, reviewing at `e42cc19d`: only the lower edge was enforced.

    if when < cutoff:
        continue

A parseable future timestamp therefore counted as an overnight DM in every
briefing until the wall clock caught up. Reproduced against the real function
in an isolated workspace, with an owner DM at
`tasks/archive/2026-08/task-future.txt` stamped 2028-01-15:

    get_overnight_discord(now=1800000000) -> ['future-dated']

The impact is worse than an extra row, and this is the part the review did not
state: `found[-5:]` slices AFTER sorting, so a future stamp sorts LAST and
EVICTS a real overnight DM. With five genuine DMs plus one future-dated:

    ['real3', 'real2', 'real1', 'real0', 'future']      <- real4 is gone

So the unbounded upper edge reintroduces the false-CLEAN this PR exists to fix,
for whichever DM it displaces.

The concern is not theoretical now that briefing truth rests on mutable on-disk
stamps. Clock skew, a hand-edited file, or imported state all produce one — and
on this very workspace today, 21 fuzzed timestamps in tracked notes were found
stamped into the future.

Fix: `if not cutoff <= when <= now`. Both bounds inclusive — `cutoff` is 8h ago
exactly, and a DM arriving this instant must still count.

Tests: 23 pass (16 -> 23). The reviewer asked for a future-dated regression in
the month-partitioned archive; testing the whole boundary AXIS instead, since a
fixture set containing only the case a reviewer named cannot catch the next one:

    future-dated in tasks/archive/YYYY-MM/     ) all four FAIL at e42cc19
    future-dated in flat tasks/archive/        )
    future-dated in live tasks/                )
    future does not displace real DMs          )
    written exactly now         -> included    ) pass BOTH ways on purpose:
    exactly at cutoff           -> included    ) guards against over-correcting
    one second before cutoff    -> excluded    ) the bound

One fixture note worth keeping: the at-cutoff case is pinned to a whole second.
`write_task` serialises with `%S`, so a fractional `now` truncates the stamp
BELOW a fractional cutoff and the case silently degrades into "one second
early" rather than "exactly at the edge" — the fixture format cannot otherwise
express the boundary being asserted. It failed first for that reason, not
because the bound was wrong.

All 15 test files touching morning-briefing pass.

Stand: Echo Act IV Mini

---------

Co-authored-by: Rui Wang <52230987+john-the-dev@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@sonichi