Skip to content

fix(fsapp): drop on_final from turn hook so exit_reason no longer finalizes the task (#685) - #760

Open
Kailigithub wants to merge 1 commit into
lsdefine:mainfrom
Kailigithub:fix/issue-685-fsapp-exit-reason
Open

fix(fsapp): drop on_final from turn hook so exit_reason no longer finalizes the task (#685)#760
Kailigithub wants to merge 1 commit into
lsdefine:mainfrom
Kailigithub:fix/issue-685-fsapp-exit-reason

Conversation

@Kailigithub

Copy link
Copy Markdown
Contributor

Summary

_make_task_hook in frontends/fsapp.py previously treated any turn-level exit_reason as completion of the whole Feishu task and invoked the task's on_final callback (_finish) immediately. That callback sets result["sent"] = True, so run_agent could leave its display-queue loop before the queue published its real {"done": ...} item. The card could be marked complete from an intermediate turn response, and final generated-file/attachment handling could run with the wrong text.

The same hook also had a second bug: when a single context contained both exit_reason and summary, the exit_reason branch won, so the per-turn step was not added to the card.

This PR makes the hook summary-only — it only patches the per-turn card step from summary. Finalization is now reachable only via the display-queue done item (and the timeout/stop/exception paths in run_agent, which were unchanged).

Fixes #685.

Root cause

_make_task_hook(card, task_id, on_final) was passed _finish as on_final. The inner hook then fired on_final whenever ctx['exit_reason'] was truthy. Because exit_reason is a per-turn signal (see agent_loop.py:73-92exit_reason is set inside the per-turn outcome loop, not the per-task one), an intermediate turn-end would prematurely finalize the queued task.

Reproduction

Issue #685 includes an isolated harness exercising _make_task_hook:

[01_exit_reason_with_response_triggers_on_final]
expect={'on_final': 1, 'step': 0, 'fail': 0}
actual={'on_final': 1, 'step': 0, 'fail': 0}

[05_summary_and_exit_reason_together_exit_reason_wins]
expect={'on_final': 1, 'step': 0, 'fail': 0}
actual={'on_final': 1, 'step': 0, 'fail': 0}
on_final_arg='final text'

After this fix, both on_final calls disappear (finalization belongs to the queue's done item) and the summary is preserved as a card step.

Change

  • _make_task_hook(card, task_id, on_final)_make_task_hook(card, task_id)
  • Inner hook drops the exit_reason branch and the on_final(raw) invocation. The summary branch is unchanged.
  • Caller in run_agent no longer passes _finish. The display-queue loop's if item and "done" in item: await asyncio.to_thread(_finish, item.get("done", "")) path is the sole _finish entrypoint (alongside timeout/stop/exception, which were untouched).

Net diff: 12 insertions, 8 deletions in frontends/fsapp.py.

Test

tests/test_fsapp_exit_reason_no_finalize.py covers:

  1. exit_reason + response → no _finish, no step (the core [fsapp] turn-end exit_reason can finalize a queued task before display queue done #685 case).
  2. summary + exit_reason → step is recorded, no _finish.
  3. summary alone → step is recorded, no _finish.
  4. stale task_id → hook is a no-op.
  5. _make_task_hook signature no longer carries on_final.
  6. behavioral pin of the old on_final branch — keeps the regression test honest if fsapp.py ever regains an on_final path.

The tests load the live _make_task_hook AST source so they exercise the actual upstream function (not a copy) and can be run with python tests/test_fsapp_exit_reason_no_finalize.py on a fresh clone (no pytest, no node_modules, no Feishu credentials).

Verification

python tests/test_fsapp_exit_reason_no_finalize.py
# PASS test_hook_does_not_finalize_on_exit_reason_with_response
# PASS test_hook_adds_step_when_summary_present_with_exit_reason
# PASS test_hook_adds_step_for_summary_only
# PASS test_hook_is_no_op_for_stale_task_id
# PASS test_hook_signature_no_longer_takes_on_final
# PASS test_old_signature_would_have_finalized_on_exit_reason
# ALL PASS

python -m py_compile frontends/fsapp.py

The same suite was also run against the unfixed source (git checkout main -- frontends/fsapp.py) and correctly reports the missing on_final argument / old signature.

Closes #685

…alizes the task (lsdefine#685)

A turn-level exit_reason can fire on an intermediate agent turn, not only
on the queued task's real completion. The previous _make_task_hook used
that signal to call _finish early, which set result['sent'] = True
before the display queue published its real {done: ...} item. Cards
could therefore be marked complete with the wrong text and final
attachment handling could run against an unfinished task.

Make the hook summary-only: it patches the per-turn card step from
summary and never finalizes the task. Finalization now lives only on
the display-queue done item (and on the timeout/stop/exception paths
in run_agent, which were unchanged).

Adds tests/test_fsapp_exit_reason_no_finalize.py with 6 cases that
exercise the live _make_task_hook plus a behavioral pin of the old
on_final branch.

Fixes lsdefine#685.
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.

[fsapp] turn-end exit_reason can finalize a queued task before display queue done

1 participant