fix: separate file edit progress ids

fix: separate file edit progress ids in HKUDS/nanobot.

Open

hamb1ywants to merge intofrom

Summary

keep provider tool_call IDs immutable after final response parsing
add a separate file-edit progress_id used only to correlate streamed placeholders with final file-edit events
update WebUI file-edit merging to prefer progress_id for file-edit event coalescing while keeping call_id for provider/tool trace correlation

Fixes #4603

uv run pytest tests/utils/test_file_edit_events.py -q
uv run ruff check nanobot/utils/file_edit_events.py nanobot/agent/runner.py nanobot/providers/base.py tests/utils/test_file_edit_events.py
cd webui && bun run build

Targeted Vitest command bun run test -- useNanobotStream currently fails before collecting tests in this environment because localStorage is undefined in src/tests/setup.ts.

9475ce1Jul 2

hamb1y-bot-hkuds-nanobot4mo ago

What this PR does: Implements the #4603 refactor properly:

Adds progress_id: str | None to FileEditTracker dataclass.
Adds progress_id_for(tool_call) method on StreamingFileEditTracker that returns the canonical ID without mutating the tool call.
Makes apply_final_call_ids a deprecated no-op (preserved for compat).
Threads file_edit_progress_id through _execute_tools → _run_tool → prepare_file_edit_tracker.
Adds file_edit_progress_ids: list[str | None] field to LLMResponse.
Runner now populates response.file_edit_progress_ids = [live_file_edits.progress_id_for(tc) for tc in response.tool_calls] instead of mutating IDs.
WebUI updated to prefer progress_id over call_id for file-edit event merging (fileEditKey, fileEditProgressKey, fileEditCallKey).

Provider tool_call.id is now immutable after final response parsing. This is the core invariant #4603 asked for.
Separate correlation key (progress_id) is the right abstraction. It's carried in file-edit events only and doesn't pollute provider protocol state.
Deprecated apply_final_call_ids is kept as a no-op rather than removed — defensive against any external callers.
WebUI changes are minimal and consistent. progress_id || call_id fallback handles old events gracefully.
Test coverage is thorough: progress_id_for returns canonical, final.id stays as provider-final-id, multiple file-edit tools with different IDs, vitest test for the WebUI merge path.

Vitest note in PR body is concerning. "Targeted Vitest command bun run test -- useNanobotStream currently fails before collecting tests in this environment because localStorage is undefined in src/tests/setup.ts." The author is honest about this, but it means the new WebUI test (upgrades streamed file_edit placeholders by progress_id) wasn't actually run locally. CI must verify it passes.
progress_id defaults to call_id in build_file_edit_pending_event: "progress_id": str(call_id or ""). This is correct for streamed placeholders (where call_id is the early ID) but means old clients that don't know about progress_id can still correlate via call_id. Good backwards-compat.
apply_final_call_ids should arguably log a deprecation warning when called, to surface any stale callers. Currently silent. Minor.
id(tool_call) is used as the dict key in progress_id_by_call to look up the progress ID per tool call. This works because tool_call objects are unique instances, but if any caller copies the list (e.g., list(tool_calls)) the IDs would differ. Verify the runner doesn't copy between _execute_tools invocation and the _run_tool call. Looking at the diff, the same tool_call objects flow through, so this is safe — but fragile to future refactors.

Approve. This is the cleanest of the architectural PRs in this batch. The only blocker is verifying the vitest test actually passes on CI.

hamb1y-bot-hkuds-nanobot4mo ago

What this PR does: Implements the #4603 refactor properly:

Adds progress_id: str | None to FileEditTracker dataclass.
Adds progress_id_for(tool_call) method on StreamingFileEditTracker that returns the canonical ID without mutating the tool call.
Makes apply_final_call_ids a deprecated no-op (preserved for compat).
Threads file_edit_progress_id through _execute_tools → _run_tool → prepare_file_edit_tracker.
Adds file_edit_progress_ids: list[str | None] field to LLMResponse.
Runner now populates response.file_edit_progress_ids = [live_file_edits.progress_id_for(tc) for tc in response.tool_calls] instead of mutating IDs.
WebUI updated to prefer progress_id over call_id for file-edit event merging (fileEditKey, fileEditProgressKey, fileEditCallKey).

Provider tool_call.id is now immutable after final response parsing. This is the core invariant #4603 asked for.
Separate correlation key (progress_id) is the right abstraction. It's carried in file-edit events only and doesn't pollute provider protocol state.
Deprecated apply_final_call_ids is kept as a no-op rather than removed — defensive against any external callers.
WebUI changes are minimal and consistent. progress_id || call_id fallback handles old events gracefully.
Test coverage is thorough: progress_id_for returns canonical, final.id stays as provider-final-id, multiple file-edit tools with different IDs, vitest test for the WebUI merge path.

Vitest note in PR body is concerning. "Targeted Vitest command bun run test -- useNanobotStream currently fails before collecting tests in this environment because localStorage is undefined in src/tests/setup.ts." The author is honest about this, but it means the new WebUI test (upgrades streamed file_edit placeholders by progress_id) wasn't actually run locally. CI must verify it passes.
progress_id defaults to call_id in build_file_edit_pending_event: "progress_id": str(call_id or ""). This is correct for streamed placeholders (where call_id is the early ID) but means old clients that don't know about progress_id can still correlate via call_id. Good backwards-compat.
apply_final_call_ids should arguably log a deprecation warning when called, to surface any stale callers. Currently silent. Minor.
id(tool_call) is used as the dict key in progress_id_by_call to look up the progress ID per tool call. This works because tool_call objects are unique instances, but if any caller copies the list (e.g., list(tool_calls)) the IDs would differ. Verify the runner doesn't copy between _execute_tools invocation and the _run_tool call. Looking at the diff, the same tool_call objects flow through, so this is safe — but fragile to future refactors.

Approve. This is the cleanest of the architectural PRs in this batch. The only blocker is verifying the vitest test actually passes on CI.