Skip to content

Dart regen script: Windows paths for native tools; antd-go unsatisfiedSize table test - #320

Merged
Nic-dorman merged 2 commits into
mainfrom
nic/v2-1520-1521-regen-paths-and-416-tests
Oct 13, 2026
Merged

Nic-dorman merged 2 commits into
mainfrom
nic/v2-1520-1521-regen-paths-and-416-tests

Conversation

@Nic-dorman

@Nic-dorman Nic-dorman commented Oct 13, 2026 •

Copy link
Copy Markdown
Member

Fixes two non-blocking findings from the review of #318 (now merged).

Dart regen script on Windows (antd-dart/tool/generate_proto.sh)

The review flagged that the plugin path wasn't converted for native protoc. Testing on Windows showed a wider problem. Git Bash converts MSYS paths (/c/Users/...) for native programs, but MSYS_NO_PATHCONV=1 turns that off, and then:

  • protoc can't find the proto root (antd/proto: warning: directory does not exist), so converting the plugin path alone doesn't help.
  • dart can't find a PUB_CACHE given in MSYS form.
  • The script had already deleted the committed stubs before protoc ran, so the failed run left them deleted.

This PR:

  • converts PUB_CACHE, the proto root, the proto files and the plugin with cygpath -m, as the script already did for the output dir;
  • generates into a temp dir and replaces the stubs only after protoc succeeds.

The review's other concern was a backslash PUB_CACHE (C:\...) failing the plugin -f check. That doesn't happen: Git Bash's [[ -f ]] accepts it.

On Linux and macOS the conversion is a no-op; only the temp dir is new there.

antd-go unsatisfiedSize unit test

The unit check covered only bytes */1234 and "". It is now a table that also covers:

  • a missing, non-numeric or negative size;
  • a satisfiable bytes 0-1/10, another unit, and surrounding whitespace.

Testing

  • Windows, Git Bash, protoc 34.0, protoc_plugin 22.3.0:
    • zero content drift with and without MSYS_NO_PATHCONV=1, and with PUB_CACHE in MSYS or backslash form;
    • with a protoc that fails, the script exits 1 and all 23 stubs stay in place;
    • the temp dir is removed in every case.
  • Lint: shellcheck 0.10.0 is clean at style level.
  • antd-go: go test ./..., go vet, gofmt and golangci-lint v1.64.8 are all clean.
  • CI runs the Dart drift job on Linux.

🤖 Generated with Claude Code

Nic-dorman and others added 2 commits October 13, 2026 09:45
Git Bash converts MSYS paths for native programs, but not with
MSYS_NO_PATHCONV=1. Then protoc cannot find the proto root, and dart
cannot find a PUB_CACHE given in MSYS form. Convert PUB_CACHE, the proto
root, the proto files and the plugin with cygpath -m, as the script
already did for the output dir.

Also generate into a temp dir and replace the stubs only after protoc
succeeds, so a failed run no longer deletes the committed stubs.

Tested on Windows (Git Bash, protoc 34.0, protoc_plugin 22.3.0):
- zero drift with and without MSYS_NO_PATHCONV=1, and with PUB_CACHE in
  MSYS or backslash form
- a failing protoc leaves all 23 stubs in place

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The unit check covered only a valid value and an empty one; a malformed
size was exercised only through the gRPC mock. Cover a missing, non-
numeric or negative size, a satisfiable range, another unit, and
surrounding whitespace.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Nic-dorman
Nic-dorman changed the base branch from nic/range-review-followups to main October 13, 2026 08:51
@Nic-dorman
Nic-dorman merged commit db42419 into main Oct 13, 2026
12 checks passed
@dirvine

dirvine commented Oct 13, 2026

Copy link
Copy Markdown
Member

Retrospective technical review — provisional

Reviewed WithAutonomi/ant-sdk PR #320 at head c51409510f36156339e06b309c2165ec74f94a4a. The PR is already merged. This is a technical review comment, not a formal approval or a completed full-team review gate.

Result: The available reviewers found no blocking defect. The Windows path changes and the Go table test are reasonable. There is one non-blocking failure-handling gap and one verification gap.

Actual panel participation

Two independent role passes ran against the same revision and evidence: correctness/evidence, then adversarial/failure modes. The second pass did not receive the first pass's conclusions.

  • Qwen 3.8 (qwen38-27b, local): completed both passes.
  • DS4 Flash (deepseek-v4-flash, local): completed both passes.
  • GLM 5.3 (zai-org/GLM-5.3, Hugging Face): failed both attempts with HTTP 402, no remaining inference credits. No substitute was counted.

2/3 models reviewed — provisional; 4/6 requested role reviews completed. DS4 synthesis is not an additional independent vote. There is no full-team consensus claim.

Findings

1. Non-blocking: replacement is not atomic or safe against every failure.

generate_proto.sh:79–83 deletes old stubs before copying the new files. The temporary generation directory protects committed stubs when protoc exits nonzero. A later copy failure can still leave the output missing or incomplete. Both local models flagged this gap in the adversarial pass.

A separate, deliberately abnormal Unix fixture made fake protoc exit successfully without producing files. At the PR head, the script removed the old stub, then failed at cp with exit 1. The same fixture against the merge base also removed the old stub, but exited 0. This confirms that the delete-before-replace hazard predates the PR; the new copy stage does not eliminate it. It does not show that real protoc_plugin produces empty output for the supplied valid inputs.

Follow-up: validate the expected generated files before deletion and add a copy-failure test. If full failure safety is required, use a rollback-safe installation step. Simply moving cp before rm is not an atomic fix: a partial copy can already overwrite files.

2. Verification gap: real Windows behaviour remains author-reported.

The path conversions and quoting are consistent with the intended Git Bash/native-tool boundary. However, the supplied CI does not run this script on Windows. The local fixtures use fake Dart/protoc on Unix; they do not verify real cygpath, .bat plugin execution or Windows paths with spaces.

A repeatable Windows run with spaces in both the repository path and PUB_CACHE, plus a generator-failure case, would close this gap.

Go test: TestUnsatisfiedSize covers valid sizes, zero, surrounding whitespace, missing/non-numeric/negative sizes, satisfiable ranges and the wrong unit. No defect found in this change.

Verification

  • Live GitHub status: 10 successful checks, 2 skipped. The skipped race/security jobs must not be counted as passed.
  • Local bash -n, git diff --check, targeted TestUnsatisfiedSize, full Go suite and go vet: passed.
  • Unix fake-tool fixtures: successful replacement with spaces and partial generator failure passed their assertions, including cleanup and preservation of old stubs on nonzero generator exit.
  • No local real Dart/protoc or Windows execution is claimed.

Reconciliation

Qwen's first pass incorrectly said copy failure occurs before deletion; the source and fixture show the reverse. The adversarial reviews also differed over whether to call the remaining replacement hazard introduced or inherited. The base/head fixture establishes the inherited deletion hazard while preserving the concern about the new copy step. DS4's proposed copy-first ordering was not accepted as an atomic solution.

Next step: treat these as provisional technical findings. Retry the missing GLM 5.3 reviews after Hugging Face inference access is restored before calling this a full-team review.

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