Skip to content

fix: preserve runtime status, verification, and response identities - #270

Open
dajiaohuang wants to merge 4 commits into
HKUDS:mainfrom
dajiaohuang:c5/preserve-runtime-contracts-20261016
Open

dajiaohuang wants to merge 4 commits into
HKUDS:mainfrom
dajiaohuang:c5/preserve-runtime-contracts-20261016

Conversation

@dajiaohuang

@dajiaohuang dajiaohuang commented Oct 18, 2026 •

Copy link
Copy Markdown

Description

This PR submits three independently reproduced runtime fixes preserved on the existing fork branch.

Related Issues

No directly matching open issue or pull request was found in the scoped current-object reconciliation. This does not duplicate #252, whose dependency change is already equivalent to upstream commit 3d40e25.

Changes Made

  • Preserve failing exit status when the schedule monitor emits JSON errors.
  • Normalize Windows verification launcher paths so npm tests launch from a project path containing spaces, retaining the active Python interpreter.
  • Preserve provider call/item IDs through Responses conversion and assign non-colliding fallback item IDs, without changing Chat normalization.

Checklist

  • Changes tested locally
  • Code reviewed
  • Documentation updated (if necessary)
  • Unit tests added (if applicable)

Additional Notes

Validation on the saved candidate: 92 focused Windows/Python 3.13 tests passed, including CLI/npm/SQLite fixtures and a local provider transport roundtrip; changed-file Ruff format/check and staged whitespace checks passed. Full suite, Linux matrix, package/build, frozen Desktop and live-provider acceptance were not run. No merge or CI acceptance is claimed.

@Zongwei9888

Copy link
Copy Markdown
Collaborator

Thank you for this, and welcome! We went through the three fixes one by one on main:

  1. Monitor exit code (core/schedule/monitor.py): correct. With --json, a failed script or URL exited 0 on main while plain mode exited 1; your test fails on main and passes with the change.
  2. Responses tool-call ids (openai_compat.py, converters.py): correct, and a real bug. On main, two tool calls in one reply are both sent back as fc_1 on the next turn. Keeping the original ids is the right fix. One small leftover: when a gateway omits the item id, core/providers/openai_responses/parsing.py (lines 86, 128, 178, 267, 315) falls back to "fc_0", which is now sent back unchanged, so every such call would share fc_0. Dropping that fallback would let the converter's unique fc_{idx}_{tool_idx} apply.
  3. Verification launcher (core/verification.py): this one would break the Test button for most users, so we can't merge it as is. normalize_stdio_command also maps python/python3 to sys.executable (core/platform_compat.py:124). That mapping exists for MCP servers; here it would run the user's project tests with DeepCode's own interpreter. With the README install (uv tool install), that environment has neither pytest (only in the test extra, setup.py:75) nor the project's dependencies. We reproduced it with a sample project: main runs python3 from PATH and prints 1 passed; with this PR it fails with No module named pytest. The Windows part is valid, though: on main npm test can't launch because npm is npm.cmd.

What we'd merge: keep the Windows .cmd/.bat wrapping in run_verification but skip the python-to-interpreter mapping there (for example a keyword argument on normalize_stdio_command), and drop or invert test_verification_uses_the_active_python_interpreter. If you'd rather, split fixes 1 and 2 into their own PR and we'll merge them right away; they're ready as they are.

@dajiaohuang

Copy link
Copy Markdown
Author

Thanks for the detailed review. I addressed the remaining feedback in commit 2ff01d8 on this PR. Responses parsing now omits synthetic fc_0 IDs in non-stream, SSE, and SDK-stream paths when an item ID is absent; the converter supplies unique per-message/per-tool IDs. Added offline regressions for all three parser paths and a two-call roundtrip. Verification now disables only bare python/python3 remapping to DeepCode's interpreter; Windows .cmd/.bat wrapping remains. Replaced the active-interpreter assertion with a PATH-owned project-python regression; the npm launcher test remains. Validation: 28 focused tests passed, Ruff format/check passed on the seven changed files, and git diff --check passed. No live providers were used. GitHub currently reports no status checks for this head; the PR remains mergeable. No CI or merge acceptance is claimed.

This branch has not been deployed

No deployments
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.

2 participants