Files
50292cc754 fix: centralize command validation in corecmd (#1292)
* fix: normalize command validation errors

* fix: normalize command validation errors

* fix: preserve non-validation pre-run errors

* fix: preserve non-validation pre-run errors

* fix: centralize command validation lifecycle

* fix: preserve validation across Cobra traversal and proxies

* chore: preserve upstream Cobra source formatting

* ci: bind Cobra compatibility checks to the PR head

* docs: record completed validation architecture review

* fix: preserve validation failure ownership and lock regression boundaries

* fix: type native Cobra validation for generated commands

* fix: classify legacy Cobra argument lookup failures

* test(app): close the file logger before Windows temp dir cleanup

Executing the runtime root opens <config dir>/logs/dws.log and keeps that
handle for the process lifetime. With DWS_CONFIG_DIR pointed at a
t.TempDir(), Windows cannot remove the directory while the handle is open,
so TestCrossPlatformCoverageTypedValidationErrorGateExtensions failed only
on the Windows coverage job while passing everywhere else.

Register CloseFileLogger after the TempDir so LIFO cleanup releases the
handle first, matching the convention the credential and skill tests use.

* test: cover the fail-closed branches the platform coverage gate counts

The macOS and Windows gates require 100% coverage of changed statements
but only execute TestCrossPlatformCoverage*/TestAllShortcuts* tests, so
three changed statements stayed uncovered even though the full suite passed:

- ResultInvoke rejected without an active unified-result rollout was already
  tested, only under a name the gate filter skips. Rename it.
- ExecuteCForTest propagating a PrepareCommandTree failure, reachable when
  the root itself is unprepared but a descendant already is.
- Root assembly panicking instead of returning a half-adapted tree when a
  mount has already been prepared.

* test(corecmd): cover the ExecuteContext*ForTest success path in package

The aggregate coverage gate assembles per-shard profiles whose -coverpkg is
derived from each shard's changed packages, so a statement in internal/corecmd
exercised only by internal/app or internal/helpers callers can stay uncovered
in the union even though the platform gate, which instruments all four packages
at once, reports 100%. ExecuteContextCForTest's SetContext-and-delegate path
was in that position: in-package tests only reached its nil guard.

Cover it from inside the package so the statement no longer depends on
cross-package instrumentation.

* test(helpers): run standalone whiteboard tests through the prepared tree

Merging main brought in nine new whiteboard tests that execute through bare
cmd.Execute(). A standalone Cobra execution never installs the framework's
validation adapters, so those tests exercised the unprepared path while the
nine pre-existing tests in the same file already used corecmd.ExecuteForTest.

Route all of them through ExecuteForTest so the whole file asserts against the
prepared tree, matching the contract that standalone command tests use the
corecmd *ForTest helpers. Every migrated site used only the returned error, so
the change is one-to-one.

* docs: require standalone command tests to run through the prepared tree

AGENTS.md said standalone command tests may use the corecmd *ForTest helpers.
Permissive wording let a merge from main bring in nine whiteboard tests that
execute through bare cmd.Execute(), which never installs the preparation-stage
validation adapters, so they asserted against the un-adapted path and a
parameter-validation regression would have passed silently.

Make the helper mandatory, record why bare execution is unsafe, and state that
tests arriving from main are in scope so a merge has to re-check them.

* docs: scope the ForTest requirement to test-constructed commands

The rule as first written also flagged root.Execute() after NewRootCommand(),
which is correct code: the app factory already prepared that tree, so bare
Execute runs the adapted path. An over-broad rule forces pointless churn and
cries wolf on valid tests, so exempt factory roots and name the real risk,
which is a tree the test built itself and never prepared.

* fix(errors): keep deadline classification at the business error boundary

WrapErrorWithOperation switched its pass-through guard from "is a structured
*apperrors.Error" to PreserveClassification. That predicate also returns true
for the cancellation and deadline sentinels, so a real context.DeadlineExceeded
left the wrapper untouched, never reached the network-timeout branch, and fell
through to apperrors.ExitCode as internal/exit 5 — losing NETWORK_TIMEOUT, the
API exit code and the retry hint. resolveFileDomain now preserves and wraps
deadlines explicitly, so genuine request timeouts hit this reliably.

Split the concept instead of reverting it, because both behaviours are correct
in their own place. DeclaresClassification recognises only errors carrying a
contract of their own, a structured *Error or an ExitCoder, and is what a
classification boundary should use. PreserveClassification keeps adding
cancellation and deadline identity for validation boundaries, where a timeout
must never be rewritten as a parameter failure.

The existing message table did not catch this: it feeds errors.New(text), and
errors.Is matches only the real sentinel. The regression was in fact pinned by a
test asserting that a wrapped deadline passes through unchanged, so that
assertion is corrected and the sentinel is now tested bare and wrapped, with a
negative control confirming it fails against the old predicate.

* docs(pr): add drive/errors/leaf/oa/recruit/wiki CI evidence

Add a local command-CI collage for PR #1292 covering the Auto CR
required domains, generated from passing focused tests on 7cb5c4dc.

Co-authored-by: john <typefield@users.noreply.github.com>

* test(helpers): compare whiteboard export path via JSON on Windows

Coverage (Windows) failed TestCrossPlatformCoverageWhiteboardExportDownloadsUsingBoardName
because JSON encodes backslashes, so a raw filepath substring no longer matches.

Co-authored-by: john <typefield@users.noreply.github.com>

* Sync merge tree rename deletions

* fix(test): keep result-store installation store-only; tidy go.sum

EmitStoredResult must stay with the caller: unified-result tests set the
output writer after execution and emit themselves, so an automatic emit
inside ExecuteCForTest wrote to the default writer and consumed the
store's emitAttempted, leaving captured stdout empty. go mod tidy drops
cobra v1.10.2 checksums orphaned by the merge (Policy tidy gate).

---------

Co-authored-by: 玉澜 <yulan.wqy@alibaba-inc.com>
Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: john <typefield@users.noreply.github.com>
2026-09-17 17:17:01 +08:00

108 lines
3.7 KiB
Go

// Copyright 2026 Alibaba Group
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package errors
import (
"context"
stderrors "errors"
"fmt"
"testing"
)
type validationTestExitError struct{}
func (validationTestExitError) Error() string { return "explicit" }
func (validationTestExitError) ExitCode() int { return ExitCodeAuth }
func TestCrossPlatformCoverageNormalizeValidation(t *testing.T) {
raw := stderrors.New("bad argument")
err := NormalizeValidation(raw, WithReason("invalid_parameters"))
var typed *Error
if !stderrors.As(err, &typed) {
t.Fatalf("NormalizeValidation() = %T, want *Error", err)
}
if typed.Category != CategoryValidation || typed.Reason != "invalid_parameters" || typed.Cause != raw {
t.Fatalf("NormalizeValidation() = %#v", typed)
}
if !stderrors.Is(err, raw) {
t.Fatal("NormalizeValidation() lost its cause")
}
apiErr := NewAPI("upstream failed")
if got := NormalizeValidation(apiErr); got != apiErr {
t.Fatalf("typed error changed: got %p want %p", got, apiErr)
}
exitErr := validationTestExitError{}
if got := NormalizeValidation(exitErr); got != exitErr {
t.Fatalf("ExitCoder changed: got %#v want %#v", got, exitErr)
}
for _, preserved := range []error{context.Canceled, context.DeadlineExceeded} {
if got := NormalizeValidation(preserved); got != preserved {
t.Fatalf("context error changed: got %v want %v", got, preserved)
}
}
if NormalizeValidation(nil) != nil {
t.Fatal("NormalizeValidation(nil) must be nil")
}
}
func TestCrossPlatformCoveragePreserveClassification(t *testing.T) {
for _, err := range []error{nil, stderrors.New("raw validation error")} {
if PreserveClassification(err) {
t.Fatalf("unclassified error protected: %v", err)
}
}
for _, cause := range []error{NewAPI("api"), validationTestExitError{}, context.Canceled, context.DeadlineExceeded} {
err := fmt.Errorf("outer: %w", fmt.Errorf("inner: %w", cause))
if !PreserveClassification(err) || NormalizeValidation(err) != err {
t.Fatalf("wrapped error identity lost: %v", err)
}
if ExitCode(err) != ExitCode(NormalizeValidation(err)) {
t.Fatal("exit code changed")
}
}
}
func TestCrossPlatformCoverageDeclaresClassification(t *testing.T) {
for _, err := range []error{
nil,
stderrors.New("raw business error"),
context.Canceled,
context.DeadlineExceeded,
fmt.Errorf("outer: %w", context.DeadlineExceeded),
} {
if DeclaresClassification(err) {
t.Fatalf("error carrying no contract of its own claimed one: %v", err)
}
}
for _, err := range []error{
NewAPI("api"),
validationTestExitError{},
fmt.Errorf("outer: %w", fmt.Errorf("inner: %w", NewAPI("api"))),
fmt.Errorf("outer: %w", validationTestExitError{}),
} {
if !DeclaresClassification(err) {
t.Fatalf("error carrying its own contract was not recognized: %v", err)
}
}
// The two predicates differ exactly on the cancellation and deadline
// sentinels: a validation boundary preserves their identity, while a
// classification boundary still owns them and must classify them itself.
for _, err := range []error{context.Canceled, context.DeadlineExceeded} {
if !PreserveClassification(err) || DeclaresClassification(err) {
t.Fatalf("predicate boundary drifted for %v", err)
}
}
}