Classify an error code by the failure's origin

cmd/attachmentupload/attachmentupload.go

@@ -101,7 +101,7 @@ func run(cmd *cobra.Command, args []string) error {
 
 	actions, err := plan(c, pageID, attachments)
 	if err != nil {
-		return operationalFail(pageID, err, jsonout.CodeFor(err), roots)
+		return operationalFail(pageID, err, planCode(err), roots)
 	}
 	return report(actions, roots)
 }
@@ -146,6 +146,19 @@ func forced(actions []client.SyncAction) []client.SyncAction {
 // collision or a directory passed where a file was meant.
 type badInput struct{ error }
 
+// planCode maps a plan failure to its --json code. Not bare CodeFor: plan
+// checksums every local file, so an unreadable one fails here -- and this
+// command's whole input is local files. It already tells IO from VALIDATION
+// upstream (localAttachmentsCode) and used to lose the distinction one call
+// later, reporting a file it could not read as a network failure. A refused
+// listing still classifies by its status.
+//
+// Named rather than inlined so a test can assert the decision this command
+// actually makes, instead of re-deriving the same expression beside it.
+func planCode(err error) jsonout.Code {
+	return jsonout.CodeOr(err, jsonout.CodeIO)
+}
+
 // localAttachmentsCode maps a localAttachments failure to its --json code.
 func localAttachmentsCode(err error) jsonout.Code {
 	var bad badInput

cmd/attachmentupload/attachmentupload_test.go

@@ -1,13 +1,16 @@
 package attachmentupload
 
 import (
+	"net/http"
 	"os"
 	"path"
 	"path/filepath"
 	"strings"
 	"testing"
 
 	"github.com/mozilla/markfluence/internal/client"
+	"github.com/mozilla/markfluence/internal/clienttest"
+	"github.com/mozilla/markfluence/internal/jsonout"
 	"github.com/mozilla/markfluence/internal/project"
 )
 
@@ -251,3 +254,56 @@ func TestLocalAttachmentsUnusableNameReportsWhatWasTyped(t *testing.T) {
 		}
 	}
 }
+
+// TestPlanFailureCodeSeparatesServerFromLocal is the guard on the flipped
+// fallback. plan() checksums every local file, so an unreadable one is an IO
+// failure rather than the NETWORK that CodeFor answers for anything without an
+// HTTP status -- but the fallback must not swallow a real server failure on the
+// way: the attachment listing plan() makes first can be refused.
+func TestPlanFailureCodeSeparatesServerFromLocal(t *testing.T) {
+	dir := t.TempDir()
+	good := writeFile(t, dir, "img.png")
+
+	tests := []struct {
+		name    string
+		handler http.HandlerFunc
+		files   []string
+		want    jsonout.Code
+	}{
+		{
+			"a refused listing is AUTH",
+			func(w http.ResponseWriter, _ *http.Request) {
+				w.WriteHeader(http.StatusForbidden)
+				_, _ = w.Write([]byte(`{"message":"caller cannot access Confluence"}`))
+			},
+			[]string{good},
+			jsonout.CodeAuth,
+		},
+		{
+			"a file the checksum cannot read is IO",
+			func(w http.ResponseWriter, _ *http.Request) {
+				_, _ = w.Write([]byte(`{"results":[]}`))
+			},
+			[]string{filepath.Join(dir, "gone.png")},
+			jsonout.CodeIO,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			c := clienttest.New(t, tt.handler)
+			atts := make([]client.LocalAttachment, 0, len(tt.files))
+			for _, f := range tt.files {
+				atts = append(atts, client.LocalAttachment{
+					Path: f, Filename: filepath.Base(f), Source: filepath.Base(f),
+				})
+			}
+			_, err := plan(c, "123", atts)
+			if err == nil {
+				t.Fatal("plan should have failed")
+			}
+			if got := planCode(err); got != tt.want {
+				t.Errorf("code = %q, want %q (error: %v)", got, tt.want, err)
+			}
+		})
+	}
+}

cmd/create/create.go

@@ -189,12 +189,25 @@ func validationFailure(filename, message string) failure {
 // newFailure records a phase-1 error against a file, carrying over the fields of
 // a page_id failure so abort() can report them without re-fetching anything,
 // and the code the error's type implies.
+//
+// The code defaults through jsonout.CodeOr rather than to VALIDATION, because
+// phase 1 makes four kinds of server call -- checkPageID, ResolveSpaceID,
+// checkParentInSpace, checkTitleFree -- and each can fail for reasons that have
+// nothing to do with the file. A hardcoded VALIDATION reported a revoked token
+// as a defect in a file that was perfectly fine, which is the worst case
+// because a rejected credential arrives as a 404 on every v2 route and
+// GetPageOrNil deliberately does not swallow that one (#133). Every local
+// error here -- no title, no space, a taken page_id, a title clash, a parent
+// conflict -- is not a client error, so it still takes the fallback.
 func newFailure(filename string, err error) failure {
-	f := failure{filename: filename, message: err.Error(), code: jsonout.CodeValidation}
+	f := failure{filename: filename, message: err.Error(), code: jsonout.CodeOr(err, jsonout.CodeValidation)}
 	var pf *pageIDFailure
 	if errors.As(err, &pf) {
 		f.pageID, f.url = pf.pageID, pf.url
 	}
+	// After CodeOr: a converter failure is neither a request nor a plain
+	// validation error, and CONVERT is what phase 3 reported for it before the
+	// check moved into preflight.
 	var cf *convertFailure
 	if errors.As(err, &cf) {
 		f.code = jsonout.CodeConvert
@@ -551,9 +564,13 @@ func publishOne(r record, res *createResult, pageID string, version int, c *clie
 	}
 	res.url = c.PageURL(result, pageID)
 
+	// CodeOr, not CodeFor: SyncAttachments opens every asset to checksum and
+	// upload it, so a file the converter saw but cannot now read fails here --
+	// the S7 residual named above -- and CodeFor would report a local read
+	// failure as NETWORK. A server failure still classifies by its status.
 	actions, err := c.SyncAttachments(pageID, pageContent.Attachments)
 	if err != nil {
-		return res.fail(err, jsonout.CodeFor(err))
+		return res.fail(err, jsonout.CodeOr(err, jsonout.CodeIO))
 	}
 	for _, a := range actions {
 		res.attachments = append(res.attachments, jsonout.Attachment{Action: a.Action, Filename: a.Filename})

cmd/create/run_test.go

@@ -41,6 +41,18 @@ type fakeConfluence struct {
 	// created under that title -- used to test a publish-phase failure after a
 	// successful reserve.
 	failUpdateForTitle string
+	// rejectCredential makes every route answer the way the API answers a
+	// revoked token: 404 with a title that names nothing. This is the shape
+	// #133 is about -- it is not a missing page, and reporting it as one (or as
+	// a defect in the file) sends the reader to check an id that was never the
+	// problem.
+	rejectCredential bool
+	// spacesStatus, when non-zero, is the status the space lookup answers with,
+	// for a preflight server failure that is not a credential rejection.
+	spacesStatus int
+	// malformedBody makes every route answer 200 with a body that will not
+	// decode: a request failure carrying no status to classify by.
+	malformedBody bool
 }
 
 type fakePage struct {
@@ -60,8 +72,23 @@ func (f *fakeConfluence) handle(w http.ResponseWriter, r *http.Request) {
 	f.mu.Lock()
 	defer f.mu.Unlock()
 
+	if f.rejectCredential {
+		w.WriteHeader(http.StatusNotFound)
+		_, _ = fmt.Fprint(w, `{"statusCode":404,"title":"Not Found"}`)
+		return
+	}
+	if f.malformedBody {
+		_, _ = fmt.Fprint(w, `not json at all`)
+		return
+	}
+
 	switch {
 	case r.Method == http.MethodGet && r.URL.Path == "/wiki/api/v2/spaces":
+		if f.spacesStatus != 0 {
+			w.WriteHeader(f.spacesStatus)
+			_, _ = fmt.Fprint(w, `{"statusCode":403,"message":"no"}`)
+			return
+		}
 		_, _ = fmt.Fprint(w, `{"results":[{"id":"space1"}]}`)
 
 	case r.Method == http.MethodGet && r.URL.Path == "/wiki/api/v2/pages":
@@ -642,6 +669,118 @@ func TestRunConversionFailureReportsCONVERT(t *testing.T) {
 	}
 }
 
+// TestRunPreflightRejectedCredentialReportsAUTH is #133's worst case. A revoked
+// token answers every v2 route with a 404 naming nothing, and GetPageOrNil
+// deliberately does not read that as "absent" -- so checkPageID hands back the
+// *HTTPError and preflight used to stamp it VALIDATION, blaming a file that is
+// perfectly fine. Asserted alongside a genuine VALIDATION failure in the same
+// envelope, since a code that were always AUTH would pass the first half alone.
+func TestRunPreflightRejectedCredentialReportsAUTH(t *testing.T) {
+	resetOpts(t)
+	ui.SetJSON(true)
+	t.Cleanup(func() { ui.SetJSON(false) })
+	dir := t.TempDir()
+	spaceOpt = "ENG"
+	withID := write(t, dir, "withid.md", "---\ntitle: With Id\npage_id: 123\n---\nbody\n")
+	untitled := write(t, dir, "untitled.md", "---\ntitle: \"\"\n---\nbody\n")
+
+	c, fake := newFakeConfluence(t)
+	fake.rejectCredential = true
+	out, runErr := captureStdout(t, func() error {
+		return run(testCmd(t, c.SiteURL(), dir), []string{withID, untitled})
+	})
+	if runErr == nil {
+		t.Fatal("run should have failed")
+	}
+	schematest.ValidateEnvelope(t, []byte(out))
+
+	codes := resultCodes(t, out)
+	if got := codes["withid.md"]; got != string(jsonout.CodeAuth) {
+		t.Errorf("withid.md code = %q, want AUTH -- a rejected credential is not a defect in the file", got)
+	}
+	if got := codes["untitled.md"]; got != string(jsonout.CodeValidation) {
+		t.Errorf("untitled.md code = %q, want VALIDATION", got)
+	}
+}
+
+// TestRunPreflightServerFailureIsNotVALIDATION covers the other three preflight
+// calls through the space lookup: a 403 there is the server refusing, and
+// nothing about it is knowable from the file.
+func TestRunPreflightServerFailureIsNotVALIDATION(t *testing.T) {
+	resetOpts(t)
+	ui.SetJSON(true)
+	t.Cleanup(func() { ui.SetJSON(false) })
+	dir := t.TempDir()
+	spaceOpt = "ENG"
+	path := write(t, dir, "a.md", "---\ntitle: A\n---\nbody\n")
+
+	c, fake := newFakeConfluence(t)
+	fake.spacesStatus = http.StatusForbidden
+	out, runErr := captureStdout(t, func() error {
+		return run(testCmd(t, c.SiteURL(), dir), []string{path})
+	})
+	if runErr == nil {
+		t.Fatal("run should have failed")
+	}
+	schematest.ValidateEnvelope(t, []byte(out))
+
+	if got := resultCodes(t, out)["a.md"]; got != string(jsonout.CodeAuth) {
+		t.Errorf("a.md code = %q, want AUTH", got)
+	}
+}
+
+// TestRunPreflightRequestFailureWithNoStatusReportsNETWORK is the half a type
+// check on *client.HTTPError would still get wrong. doJSON builds an HTTPError
+// only once it has a status, so a dropped connection or an undecodable
+// response carries none -- and the old rule read "not an HTTPError" as "the
+// file is at fault". An undecodable 200 stands in for the whole class: a real
+// dial failure on a GET would spend the retry budget in real time, since only
+// internal/client can stub the backoff.
+func TestRunPreflightRequestFailureWithNoStatusReportsNETWORK(t *testing.T) {
+	resetOpts(t)
+	ui.SetJSON(true)
+	t.Cleanup(func() { ui.SetJSON(false) })
+	dir := t.TempDir()
+	spaceOpt = "ENG"
+	path := write(t, dir, "a.md", "---\ntitle: A\n---\nbody\n")
+
+	c, fake := newFakeConfluence(t)
+	fake.malformedBody = true
+	out, runErr := captureStdout(t, func() error {
+		return run(testCmd(t, c.SiteURL(), dir), []string{path})
+	})
+	if runErr == nil {
+		t.Fatal("run should have failed")
+	}
+	schematest.ValidateEnvelope(t, []byte(out))
+
+	if got := resultCodes(t, out)["a.md"]; got != string(jsonout.CodeNetwork) {
+		t.Errorf("a.md code = %q, want NETWORK", got)
+	}
+}
+
+// resultCodes maps each result's base filename to the code it reported,
+// skipping results that carry none (a file the batch never reached).
+func resultCodes(t *testing.T, out string) map[string]string {
+	t.Helper()
+	var env struct {
+		Results []struct {
+			File string  `json:"file"`
+			Code *string `json:"code"`
+		} `json:"results"`
+	}
+	if err := json.Unmarshal([]byte(out), &env); err != nil {
+		t.Fatalf("unmarshal %q: %v", out, err)
+	}
+	codes := map[string]string{}
+	for _, r := range env.Results {
+		if r.Code != nil {
+			codes[filepath.Base(r.File)] = *r.Code
+		}
+	}
+	return codes
+}
+
 // captureStdout runs fn with os.Stdout redirected, returning what it printed.
 func captureStdout(t *testing.T, fn func() error) (string, error) {
 	t.Helper()

cmd/fix/fix.go

@@ -115,7 +115,12 @@ func processFile(filename string, c *client.ConfluenceClient) *fixResult {
 	}
 	page, err := locatePage(mf.Frontmatter, c)
 	if err != nil {
-		return r.fail(err, locateCode(err))
+		// locatePage mixes server failures (GetPageOrNil, SearchPagesByTitle)
+		// with local ones (no page_id or title, an ambiguous title), so the code
+		// comes from the error's origin. This was fix's own locateCode, lifted
+		// into jsonout when create needed the identical rule (#133); the
+		// transport case is what a bare type check got wrong here too.
+		return r.fail(err, jsonout.CodeOr(err, jsonout.CodeValidation))
 	}
 	r.pageID = page.ID
 

cmd/fix/json.go

@@ -1,10 +1,8 @@
 package fix
 
 import (
-	"errors"
 	"fmt"
 
-	"github.com/mozilla/markfluence/internal/client"
 	"github.com/mozilla/markfluence/internal/jsonout"
 	"github.com/mozilla/markfluence/internal/ui"
 )
@@ -137,16 +135,6 @@ func summarize(results []*fixResult) map[string]int {
 	return s
 }
 
-// locateCode classifies a page-location failure: an HTTP status maps via CodeFor,
-// anything else is a frontmatter/target problem (VALIDATION).
-func locateCode(err error) jsonout.Code {
-	var he *client.HTTPError
-	if errors.As(err, &he) {
-		return jsonout.CodeFor(err)
-	}
-	return jsonout.CodeValidation
-}
-
 func nullableStr(s string) *string {
 	if s == "" {
 		return nil