Skip to content

fix(chat): mark execution dispatched + persist real session_id (#686) - #795

Merged
vybe merged 5 commits into
devfrom
AndriiPasternak31/issue-686-plan
May 12, 2026
Merged

fix(chat): mark execution dispatched + persist real session_id (#686)#795
vybe merged 5 commits into
devfrom
AndriiPasternak31/issue-686-plan

Conversation

@AndriiPasternak31

@AndriiPasternak31 AndriiPasternak31 commented May 11, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #686 — the interactive /api/chat handler was missing the mark_execution_dispatched() defense that lives in services/task_execution_service.py, so cold-start or large-prompt chat executions were being false-stamped FAILED by cleanup_service.mark_no_session_executions_failed after 60s while the agent was still healthy and processing. This is the parallel codepath of #279 (which fixed the same bug class for /task).

  • routers/chat.py:317-321 — call db.mark_execution_dispatched() before agent_post_with_retry so the no-session sweep skips the row
  • routers/chat.py:411-428 — on SUCCESS, replace the 'dispatched' sentinel with the real Claude session UUID derived from response_data → session_data → metadata (UC1, closes the observability gap so /api/chat rows show the real UUID instead of permanently reading 'dispatched')
  • Mirrors task_execution_service.py:401-410 (commit 2798ca9, issue bug: Chat-triggered executions fail with 'Silent launch failure' despite agent being healthy #279)

Tests

tests/unit/test_chat_dispatched_marker.py (3 layers):

Docs

  • docs/memory/feature-flows/cleanup-service.md — broadened the no-session sweep + mark_execution_dispatched docs to list both producers of the 'dispatched' sentinel
  • docs/memory/feature-flows/task-execution-service.md — Coverage-table note on the Interactive chat row + Step 3b "parallel codepath" sub-paragraph; service still does NOT cover /chat, only the dispatched-sentinel mechanism is now shared

Test plan

  • pytest tests/unit/test_chat_dispatched_marker.py — 3 test layers pass locally
  • CI green on PR
  • Manual: trigger a slow MCP chat (cold-start agent) and verify the execution row reaches SUCCESS with a real Claude UUID rather than being false-failed at 60s

AndriiPasternak31 added a commit that referenced this pull request May 11, 2026
@AndriiPasternak31 @claude
fix(tests): drop bare sys.modules mutations in chat dispatched-marker…
ece8a7d 
… test

The new test/unit/test_chat_dispatched_marker.py landed with two bare
sys.modules.pop() calls that exceeded the Issue #762 lint baseline
(tests/lint_sys_modules.py — added on dev). PR #795 lint job failed.

Fix:
- Module bootstrap (`for _shadow in (...): sys.modules.pop(_shadow, None)`
  + sys.path mangling): deleted entirely. tests/unit/conftest.py already
  inserts src/backend into sys.path and preloads the canonical `utils`
  package via _preload_backend_utils(); the per-file bootstrap was
  redundant. Kept the `_BACKEND` Path constant — used by the AST tests.
- Fixture teardown (`for mod in (...): sys.modules.pop(mod, None)`):
  switched to `monkeypatch.delitem(sys.modules, mod, raising=False)`.
  monkeypatch handles both the pre-yield eviction (so `from db.schedules
  import ...` re-imports against the fresh TRINITY_DB_PATH) and the
  post-yield restore at fixture teardown — same isolation guarantee,
  scoped automatically.

Verified locally:
- AST scan finds zero bare sys.modules mutations in this file
- pytest tests/unit/test_chat_dispatched_marker.py: 10 passed

Refs #686, #762

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@AndriiPasternak31 AndriiPasternak31 mentioned this pull request May 11, 2026
Merged
3 tasks
AndriiPasternak31 and others added 4 commits May 11, 2026 23:46
@AndriiPasternak31 @claude
fix(chat): mark execution dispatched + persist real session_id (#686)
70d1038 
The interactive /api/chat handler lacked the mark_execution_dispatched()
defense that lives in services/task_execution_service.py, so cold-start
or large-prompt chat executions were being false-stamped FAILED by
cleanup_service.mark_no_session_executions_failed after 60s while the
agent was still healthy and processing.

Mirrors task_execution_service.py:401-410 (commit 2798ca9, issue #279):

1. routers/chat.py:317-321 — call db.mark_execution_dispatched() before
   agent_post_with_retry so the no-session sweep skips the row.
2. routers/chat.py:411-428 — on SUCCESS, replace the 'dispatched' sentinel
   with the real Claude session UUID from response_data/session_data/metadata
   (UC1 — closes the observability gap so /api/chat rows show the real UUID
   instead of permanently reading 'dispatched').

Tests (tests/unit/test_chat_dispatched_marker.py):

- DB-level: real sqlite verifies dispatched rows survive the 60s no-session
  sweep AND NULL/'' rows are still caught.
- AST-level: parses routers/chat.py and asserts marker ordering, the
  try/except wrapper, the claude_session_id kwarg on the SUCCESS update,
  and the FAILED-path update_execution_status survive future refactors.
- Mirror invariant: routers/chat.py and task_execution_service.py must
  BOTH contain the mark_execution_dispatched call — guards against the
  recurring bug class behind #279 and #686.

Docs updated: cleanup-service.md and task-execution-service.md now document
both producers of the 'dispatched' sentinel.

Fixes #686

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@AndriiPasternak31 @claude
fix(tests): drop bare sys.modules mutations in chat dispatched-marker…
df45445 
… test

The new test/unit/test_chat_dispatched_marker.py landed with two bare
sys.modules.pop() calls that exceeded the Issue #762 lint baseline
(tests/lint_sys_modules.py — added on dev). PR #795 lint job failed.

Fix:
- Module bootstrap (`for _shadow in (...): sys.modules.pop(_shadow, None)`
  + sys.path mangling): deleted entirely. tests/unit/conftest.py already
  inserts src/backend into sys.path and preloads the canonical `utils`
  package via _preload_backend_utils(); the per-file bootstrap was
  redundant. Kept the `_BACKEND` Path constant — used by the AST tests.
- Fixture teardown (`for mod in (...): sys.modules.pop(mod, None)`):
  switched to `monkeypatch.delitem(sys.modules, mod, raising=False)`.
  monkeypatch handles both the pre-yield eviction (so `from db.schedules
  import ...` re-imports against the fresh TRINITY_DB_PATH) and the
  post-yield restore at fixture teardown — same isolation guarantee,
  scoped automatically.

Verified locally:
- AST scan finds zero bare sys.modules mutations in this file
- pytest tests/unit/test_chat_dispatched_marker.py: 10 passed

Refs #686, #762

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@ap-31 @claude
fix(chat): validate agent-supplied session_id before persist (#686)
7858702 
Reject malformed claude_session_id values from the agent-server
response before passing to db.update_execution_status. On rejection,
leave the 'dispatched' sentinel in place so the cleanup sweep stays
correct; observability is lost for that row but trust boundary on
agent-supplied data is preserved.

Refs #686

Co-Authored-By: Claude <noreply@anthropic.com>
@ap-31 @claude
chore: gitignore quota-* test residue + add CSO audit for #686 branch
b16f2d8 
- .gitignore: ignore src/backend/config/agent-templates/quota-*/
  so future test runs don't leave behind discoverable template
  directories (mirrors existing test-*/ pattern).
- docs/security-reports/cso-diff-2026-05-12.md: CSO diff audit of
  this branch vs origin/dev. Verdict: CLEAR. One LOW housekeeping
  item (the quota-* residue) addressed by this commit.

Refs #686

Co-Authored-By: Claude <noreply@anthropic.com>
@AndriiPasternak31
AndriiPasternak31 force-pushed the AndriiPasternak31/issue-686-plan branch from ece8a7d to b16f2d8 Compare May 12, 2026 00:00
@AndriiPasternak31 @claude
fix(tests): drop bare sys.modules mutations in cleanup-orphan test
9957773 
The new tests/unit/test_cleanup_unreachable_orphan.py landed with three
bare module-level sys.modules assignments (lines 64/76/79) that exceeded
the Issue #762 lint baseline. PR #795 lint job failed.

Fix: move the docker / services.docker_service / database stubs into an
autouse fixture using monkeypatch.setitem. Each test method's lazy
`from services.cleanup_service import CleanupService` and `@patch(...)`
resolution both happen after fixture setup, so the stubs land in time —
and monkeypatch handles teardown so the next test isn't poisoned.

Verified locally:
- AST scan finds zero bare sys.modules mutations in this file
- pytest tests/unit/test_cleanup_unreachable_orphan.py: 6 passed

Refs #686, #762

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@AndriiPasternak31
AndriiPasternak31 marked this pull request as draft May 12, 2026 00:12
@AndriiPasternak31
AndriiPasternak31 marked this pull request as ready for review May 12, 2026 00:20
vybe
vybe approved these changes May 12, 2026
View reviewed changes

@vybe vybe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM — surgical fix mirroring the #279 pattern, UUID validation on agent-supplied session_id is a nice addition, three-layer tests cover the sweep invariant well. Merging.

@vybe
vybe merged commit 3727ed8 into dev May 12, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vybe vybe vybe approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@AndriiPasternak31 @vybe @ap-31