Skip to content

test(coding): add native tool quality gate - #719

Open
bogdanovich wants to merge 1 commit into
mainfrom
test/coding-harness-quality-gate
Open

test(coding): add native tool quality gate#719
bogdanovich wants to merge 1 commit into
mainfrom
test/coding-harness-quality-gate

Conversation

@bogdanovich

Copy link
Copy Markdown
Owner

Summary

  • add deterministic native coding-tool quality fixtures for two provider call forms and small/large repositories
  • retain full oversized coding command output as a bounded local artifact without changing personal/gateway exec behavior
  • add an opt-in live-provider smoke and record the P2 exit evidence

Why

P2.7 requires measured evidence that the existing native read, search, patch, write, and command contracts are usable before P3 freezes their terminal presentation.

What changed

  • exercise edit correctness, stale patches, search precision, output volume, Unicode, generated/binary paths, rename/delete, cancellation, and recovery
  • persist full truncated coding command output under thread-owned scratch with mode 0600 and an artifact tag
  • document metrics, the bounded tool-contract decision, live smoke usage, and the seven-packet P2 exit record

Validation

  • make fmt
  • make lint
  • scripts/pre-push-lint.sh --changed
  • go test ./pkg/testharness/codingquality ./pkg/tools ./cmd/mintclaw/internal/coding -count=1
  • go test -race ./pkg/testharness/codingquality ./pkg/tools ./cmd/mintclaw/internal/coding -count=1
  • git diff origin/main...HEAD --check

Risks and rollback

The production behavior change is limited to oversized synchronous output from coding-mode exec. Rollback is the single PR revert; existing personal and gateway constructors do not enable artifact retention.

Review notes

Please focus on whether the two tool-call shapes and fixture thresholds are representative, whether artifact retention remains coding-only, and whether cancellation/recovery assertions are portable.

@bogdanovich bogdanovich left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Automated review by Reviewer agent.

Comment thread pkg/tools/shell.go Outdated
Comment thread pkg/testharness/codingquality/harness.go Outdated
Comment thread pkg/testharness/codingquality/harness.go
Comment thread pkg/testharness/codingquality/harness.go Outdated
@bogdanovich

Copy link
Copy Markdown
Owner Author

Automated review by Reviewer agent.

Review Summary

The coding-only scoping and bounded model context are sound, but the new retained-output path has no storage lifecycle bound and the quality gate can report success without enforcing several P2.7 claims, including exact edit/search correctness, awkward paths, and Windows cancellation/recovery.

Findings

  1. [major] Bound retained command artifacts per thread (pkg/tools/shell.go:580)

Every distinct oversized coding command now writes its complete output into persistent thread runtime state, but neither ExecTool.Close nor another lifecycle path removes or prunes these files. A long-lived thread producing unique large outputs can therefore grow runtime/tmp/command-output without limit and eventually exhaust the state volume. Add a per-thread count/byte/age bound with deterministic pruning while preserving the artifact returned by the current command.

  1. [major] Assert exact edit and search results (pkg/testharness/codingquality/harness.go:107)

These checks can pass with incorrect tool behavior: the edit assertion only requires the new expression to occur somewhere, so duplicated code or the old expression remaining is accepted; search precision only checks two named decoys, so other unexpected paths can leak into the result. Compare edit.go with exact expected bytes and parse the files-only output to assert exact set equality with the two expected paths.

  1. [major] Exercise the claimed awkward-path contract (pkg/testharness/codingquality/harness.go:274)

The P2 exit record says this gate measures awkward paths, but every fixture name is conventional ASCII without spaces, Unicode, leading dashes, or similar path hazards. The gate can therefore pass while provider argument decoding or file tools fail on the claimed path class. Add a portable path containing spaces and Unicode and exercise it through the production read/write or patch/search calls with an exact assertion.

  1. [major] Do not report Windows cancellation without running it (pkg/testharness/codingquality/harness.go:221)

On Windows the gate unconditionally marks cancellation and recovery successful. The generic synchronous cancellation test is also skipped on Windows, so this quality report cannot detect a regression in the Windows runSync interruption and subsequent-command path. Execute a Windows-compatible cancellable command plus recovery command, or add an equivalent Windows production test and derive these report fields from executed evidence instead of constants.

Coverage Notes

  • Coverage warning: 4 changed file(s) were skipped before deep review. Reasons: repo_rule_excluded.

@bogdanovich
bogdanovich force-pushed the test/coding-harness-quality-gate branch from c2e655d to 8f3d752 Compare August 13, 2026 06:16
@bogdanovich

Copy link
Copy Markdown
Owner Author

Addressed all four findings on 8f3d752d: added deterministic 7-day/32-file/64-MiB artifact pruning that protects the current result; switched edit and files-only search checks to exact comparisons; added an exact read/write round-trip for space dir/данные 🦀.txt; and now executes cancellation plus recovery on Windows instead of assigning success constants. Local make lint, changed-scope lint, targeted tests, and race tests pass.

@bogdanovich

Copy link
Copy Markdown
Owner Author

Automated review by Reviewer agent.

Review Summary

No high-confidence issues found. The four prior findings are resolved: retained coding-command artifacts are now deterministically bounded while protecting the current result, the edit/search assertions are exact, the awkward Unicode path is exercised end to end, and cancellation plus recovery now run on Windows.

The previously reported findings are resolved in the current PR head.

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.

1 participant