Skip to content

fix(core): reject negative DeepSeek coordinates - #3067

Closed
linhongyu510 wants to merge 2 commits into
web-infra-dev:mainfrom
linhongyu510:fix/deepseek-negative-coordinate-parsing
Closed

linhongyu510 wants to merge 2 commits into
web-infra-dev:mainfrom
linhongyu510:fix/deepseek-negative-coordinate-parsing

Conversation

@linhongyu510

Copy link
Copy Markdown
Contributor

Summary

  • preserve coordinate signs when parsing DeepSeek grounding responses
  • reject negative point coordinates instead of silently converting them to positive values
  • reject negative bbox coordinates in every position
  • add regression coverage while preserving supported coordinate separators

Problem

The DeepSeek coordinate parser previously extracted values with /\d+/g. As a result, a model response such as [-10, 500] was silently interpreted as [10, 500], allowing malformed grounding output to become a valid click coordinate.

The same issue affected bbox responses: a negative edge could lose its sign before the shared coordinate-range validation saw it.

Changes

The parser now preserves an optional sign while extracting integer values and rejects any negative coordinate before returning normalized values. Existing support for bracket variants and flexible separators is unchanged.

Regression tests cover negative values in both point positions and all four bbox positions.

Validation

  • DeepSeek adapter tests: 20 passed
  • Core build type-check: passed
  • Biome: 1604 files passed

@linhongyu510

Copy link
Copy Markdown
Contributor Author

CI triage: all four PRs have the same 14-job failure fingerprint. On #3066, 12/14 failed jobs explicitly report that MIDSCENE_MODEL_NAME is unavailable; the remaining e2e-web/e2e-report checks are generic failures from the same model-backed workflow. The workflows inject repository secrets/variables on pull_request, and equivalent same-repository PR runs succeed, while newer external-fork runs are action_required. This is an external-fork trust/configuration gate, not evidence that the changed code failed. This branch is currently mergeable and only one commit behind main, so I am intentionally not force-pushing just to rerun the same restricted matrix. A maintainer can validate via a trusted branch or workflow_dispatch after reviewing the diff; switching to pull_request_target would not be an appropriate workaround.

@linhongyu510

Copy link
Copy Markdown
Contributor Author

Follow-up on the CI, same fingerprint as I noted before.

The red checks here are the model-backed / device jobs (e2e-web, chrome-extension-*, android-emulator, ios-simulator, macos/windows-desktop) that depend on the injected secrets on an external-fork run — not the changed code. This PR's change is confined to packages/core (DeepSeek adapter negative-coordinate parsing) with its own unit coverage in deepseek.test.ts; there is no core unit-test job among the failures, they are all the environment-backed matrix.

I verified the same class of change on the sibling PR #3068 by running its packages/core unit file locally (33/33 passing), so the parsing logic itself is sound. A maintainer can validate via a trusted branch or workflow_dispatch; happy to rebase if that helps.

@quanru quanru left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I completed a standard review followed by a maintainability review on commit 30388e9572c4cbead493492fd132649f7446fc2f. I found no blocking issue in the change.

The sign-preserving parser keeps negative coordinates visible to validation without changing the existing supported separators. I also checked the actual adapter/codec path locally: negative values in both point positions and all four bbox positions were rejected, while supported point formats were preserved. These were focused source-level checks, not a full package or model-backed test run.

@linhongyu510

Copy link
Copy Markdown
Contributor Author

Thanks for the review and for checking the sign-preserving parser against the adapter/codec path locally, @quanru. Glad the change looks good on your end — happy to address anything else if it comes up.

Merge current main into the contributor branch and update the added bbox
validation test to use toPixelResult. The previous API caused all four
new negative-bbox cases to fail on the PR merge snapshot.

Validation:
- pnpm run lint
- pnpm exec nx build @midscene/core --parallel=1
- pnpm exec nx run @midscene/core:test -- tests/unit-test/model-adapter tests/unit-test/locate-result-codec.test.ts --pool.maxWorkers=2 (29 files, 384 tests passed)
- pnpm exec tsc -p packages/core/tsconfig.json --noEmit --pretty false
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ quanru
❌ linhongyu510
You have signed the CLA already but the status is still pending? Let us recheck it.

quanru commented Oct 21, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the contribution. We followed up with real-model testing using DeepSeek V4.1 Flash (deepseek-v4-1-flash-260910) and Midscene's production grounding prompts.

Across 24 calls covering normal controls, partially clipped controls, and missing targets, we did not observe any negative coordinates. For clipped targets, the model kept the bounding boxes within the screenshot, using x = 0 or y = 0 at the edge.

The parser change is reasonable as a defensive measure, but we could not reproduce the model behavior it targets, so we are closing this PR for now. If a real DeepSeek response containing negative coordinates becomes available, we can revisit the fix.

@quanru quanru closed this Oct 21, 2026
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.

3 participants