Fix #83: refuse the default-notebook/encrypt-by-default pair locally

cmd/assets/skill/reference.md

@@ -73,6 +73,17 @@ Containers for notes; exactly one **default** notebook per account.
 `--make-default` promotes a notebook (the prior default is demoted; you can't
 "un-default" directly — promote another). The default notebook can't be deleted.
 
+**The default notebook can never be encrypt-by-default.** Forwarded email,
+imports, and notes created with no notebook all land in the default, and none of
+those writers can client-side encrypt — so the pair is banned outright. The CLI
+refuses it locally, in both directions: `--default-encrypt` on the default
+notebook, and `--make-default` on a notebook that already encrypts. To promote an
+encrypting notebook, turn encryption off in the same command:
+
+```bash
+harbor notebooks update <id> --make-default --default-encrypt=false
+```
+
 ---
 
 ## Notes  (aliases: `note`, `n`)

cmd/notebooks.go

@@ -104,10 +104,17 @@ var notebooksUpdateCmd = &cobra.Command{
 
 Use --make-default to promote this notebook to the account default (the prior
 default is demoted automatically). There must always be exactly one default, so
-a notebook cannot be un-defaulted directly — promote a different one instead.`,
+a notebook cannot be un-defaulted directly — promote a different one instead.
+
+The default notebook can never also be encrypt-by-default: forwarded email,
+imports, and notes created with no notebook all land in the default, and none of
+those writers can encrypt. So --default-encrypt is refused on the default
+notebook, and --make-default is refused on a notebook that encrypts — unless you
+turn it off in the same command, e.g. --make-default --default-encrypt=false.`,
 	Example: `  harbor notebooks update 5b1f... --name "Work — Active"
   harbor notebooks update 5b1f... --stack Archive --public=false
-  harbor notebooks update 5b1f... --make-default`,
+  harbor notebooks update 5b1f... --make-default
+  harbor notebooks update 5b1f... --make-default --default-encrypt=false`,
 	RunE: func(cmd *cobra.Command, args []string) error {
 		c, _, err := loadClientFromConfig()
 		if err != nil {
@@ -124,6 +131,10 @@ a notebook cannot be un-defaulted directly — promote a different one instead.`
 		if len(body) == 0 {
 			return errors.New("nothing to update — pass at least one field flag")
 		}
+		// Refuse the banned pair before spending a request on a guaranteed 422.
+		if err := guardDefaultNotebookEncrypt(c, args[0], body); err != nil {
+			return err
+		}
 		data, err := c.UpdateNotebook(args[0], body)
 		if err != nil {
 			return mapNotebookError(err)
@@ -156,6 +167,11 @@ var notebooksDeleteCmd = &cobra.Command{
 }
 
 // mapNotebookError gives friendly messages for the notebook-specific codes.
+// default_notebook_cannot_encrypt is deliberately absent from the switch below.
+// Its server message is user-facing copy shared with the web app, and the CLI's
+// default renderer prints an APIError's message verbatim — so paraphrasing it
+// here is the one way to get it wrong. Pinned by
+// TestDefaultCannotEncryptServerMessageIsNotParaphrased.
 func mapNotebookError(err error) error {
 	var apiErr *client.APIError
 	if errors.As(err, &apiErr) {
@@ -171,6 +187,67 @@ func mapNotebookError(err error) error {
 	return err
 }
 
+// defaultCannotEncryptMessage is the server's own wording for the banned pair,
+// quoted rather than paraphrased.
+//
+// The rule: a notebook can never be both the account default AND encrypt-by-
+// default. The default is where forwarded email, imports, and notes created with
+// no notebook land, and none of those writers can client-side encrypt — so a
+// note the user believes is sealed would arrive there in the clear.
+//
+// The CLI refuses locally instead of letting the request 422, so the user gets the
+// reason and the fix without spending a write. A request that states both fields
+// needs no network at all; the single-flag cases still read the notebook first,
+// so offline they surface the read error rather than this refusal.
+// The wording matches app.harbor.my internal/notebooks/notebooks.go exactly
+// (issue app.harbor.my#1338); web shows the same sentence.
+const defaultCannotEncryptMessage = "the default notebook can't encrypt notes by default — forwarded email, imports, and notes with no notebook land there; encrypt a different notebook instead"
+
+// guardDefaultNotebookEncrypt refuses an update whose RESULTING state would make
+// one notebook both the default and encrypt-by-default.
+//
+// Judged on the resulting state, not on the fields present, because the server
+// judges it that way: `--make-default --default-encrypt=false` legally promotes a
+// notebook while switching encryption off, and refusing that because the request
+// mentions both flags would block the one command that fixes the situation.
+//
+// It fetches the notebook only when the answer genuinely depends on the current
+// state — when the request sets both fields, or sets neither to a banned value,
+// no round trip happens.
+func guardDefaultNotebookEncrypt(c *client.Client, id string, body map[string]any) error {
+	wantEncrypt, encryptSet := body["default_encrypt"].(bool)
+	wantDefault, defaultSet := body["is_default"].(bool)
+
+	// Turning encryption OFF can never produce the pair, whatever else changes.
+	if encryptSet && !wantEncrypt {
+		return nil
+	}
+	// Both stated in one request: decidable with no fetch.
+	if encryptSet && wantEncrypt && defaultSet && wantDefault {
+		return errors.New(defaultCannotEncryptMessage)
+	}
+	// Only one side stated; the other comes from the notebook as it stands.
+	needCurrent := (encryptSet && wantEncrypt) || (defaultSet && wantDefault)
+	if !needCurrent {
+		return nil
+	}
+	data, err := c.GetNotebook(id, false)
+	if err != nil {
+		// A notebook we cannot read is not one we can clear. Let the write go and
+		// let the server judge it — it enforces the same rule, and failing the
+		// command here would turn a transient read error into a refusal to write.
+		return nil
+	}
+	nb := parseJSON(client.UnwrapData(data))
+	if encryptSet && wantEncrypt && boolean(nb, "is_default") {
+		return errors.New(defaultCannotEncryptMessage)
+	}
+	if defaultSet && wantDefault && boolean(nb, "default_encrypt") {
+		return errors.New(defaultCannotEncryptMessage + " — switch it off with '--default-encrypt=false' in the same command to promote this notebook")
+	}
+	return nil
+}
+
 // ===========================================================================
 // Display
 // ===========================================================================
@@ -239,9 +316,9 @@ func init() {
 
 	notebooksUpdateCmd.Flags().String("name", "", "New name")
 	notebooksUpdateCmd.Flags().String("stack", "", "New stack")
-	notebooksUpdateCmd.Flags().Bool("default-encrypt", false, "Encrypt new notes by default")
+	notebooksUpdateCmd.Flags().Bool("default-encrypt", false, "Encrypt new notes by default (never allowed on the default notebook)")
 	notebooksUpdateCmd.Flags().Bool("public", false, "Make the notebook public")
-	notebooksUpdateCmd.Flags().Bool("make-default", false, "Promote this notebook to the account default")
+	notebooksUpdateCmd.Flags().Bool("make-default", false, "Promote this notebook to the account default (refused if it encrypts by default)")
 
 	notebooksDeleteCmd.Flags().String("notes", "", "What to do with its notes: move_to_default (default) or trash")
 

cmd/notebooks_test.go

@@ -4,8 +4,14 @@
 package cmd
 
 import (
+	"errors"
+	"fmt"
+	"net/http"
+	"net/http/httptest"
 	"strings"
 	"testing"
+
+	"github.com/HarborMyNotes/harbor-cli/client"
 )
 
 func TestDisplayNotebooks(t *testing.T) {
@@ -46,3 +52,149 @@ func TestMapNotebookError(t *testing.T) {
 		}
 	}
 }
+
+// nbGuardServer stands in for the API when the guard needs to read a notebook's
+// current state. It records whether the GET happened, so the tests can prove the
+// no-fetch cases really do not spend a request.
+func nbGuardServer(t *testing.T, isDefault, defaultEncrypt bool, fetched *int) *client.Client {
+	t.Helper()
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		*fetched++
+		w.Header().Set("Content-Type", "application/json")
+		fmt.Fprintf(w, `{"id":"nb1","name":"Work","is_default":%t,"default_encrypt":%t}`, isDefault, defaultEncrypt)
+	}))
+	t.Cleanup(srv.Close)
+	return client.NewClient(srv.URL, "at_test")
+}
+
+// TestGuardRefusesBothDirections pins the rule in both directions: turning
+// encryption on for the current default, and promoting a notebook that already
+// encrypts. Either way the CLI refuses before spending a request on a 422.
+func TestGuardRefusesBothDirections(t *testing.T) {
+	cases := []struct {
+		name           string
+		isDefault      bool
+		defaultEncrypt bool
+		body           map[string]any
+		wantRefused    bool
+	}{
+		{"encrypt-on for the current default", true, false,
+			map[string]any{"default_encrypt": true}, true},
+		{"promote a notebook that encrypts", false, true,
+			map[string]any{"is_default": true}, true},
+		{"encrypt-on for a non-default notebook", false, false,
+			map[string]any{"default_encrypt": true}, false},
+		{"promote a notebook that does not encrypt", false, false,
+			map[string]any{"is_default": true}, false},
+	}
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			fetched := 0
+			c := nbGuardServer(t, tc.isDefault, tc.defaultEncrypt, &fetched)
+			err := guardDefaultNotebookEncrypt(c, "nb1", tc.body)
+			if tc.wantRefused && err == nil {
+				t.Fatal("the banned pair was allowed through")
+			}
+			if !tc.wantRefused && err != nil {
+				t.Fatalf("a legal update was refused: %v", err)
+			}
+			if err != nil && !strings.Contains(err.Error(), "encrypt a different notebook instead") {
+				t.Errorf("refusal is not the server's wording: %v", err)
+			}
+		})
+	}
+}
+
+// TestGuardJudgesTheResultingState is the case the issue calls out explicitly:
+// one request carrying BOTH fields legally promotes a notebook while switching
+// encryption off. Judging on "does the request mention both flags" would block
+// the single command that fixes an encrypting notebook someone wants as default.
+func TestGuardJudgesTheResultingState(t *testing.T) {
+	fetched := 0
+	c := nbGuardServer(t, false, true, &fetched)
+
+	// Promote while switching encryption OFF — legal, and decided with no fetch.
+	if err := guardDefaultNotebookEncrypt(c, "nb1", map[string]any{"is_default": true, "default_encrypt": false}); err != nil {
+		t.Fatalf("promoting while turning encryption off was refused: %v", err)
+	}
+	if fetched != 0 {
+		t.Errorf("the resulting state was fully stated, but the guard still fetched %d time(s)", fetched)
+	}
+
+	// Both ON in one request — refused, also with no fetch.
+	if err := guardDefaultNotebookEncrypt(c, "nb1", map[string]any{"is_default": true, "default_encrypt": true}); err == nil {
+		t.Fatal("setting both flags true in one request was allowed")
+	}
+	if fetched != 0 {
+		t.Errorf("a request stating both fields needs no round trip, but the guard fetched %d time(s)", fetched)
+	}
+}
+
+// TestGuardDoesNotFetchWhenIrrelevant proves the guard costs nothing on the
+// updates that cannot produce the pair — renames, stack moves, and any request
+// that turns encryption off.
+func TestGuardDoesNotFetchWhenIrrelevant(t *testing.T) {
+	for name, body := range map[string]map[string]any{
+		"rename only":     {"name": "Work — Active"},
+		"encryption off":  {"default_encrypt": false},
+		"off and renamed": {"default_encrypt": false, "name": "x"},
+	} {
+		fetched := 0
+		c := nbGuardServer(t, true, true, &fetched)
+		if err := guardDefaultNotebookEncrypt(c, "nb1", body); err != nil {
+			t.Errorf("%s was refused: %v", name, err)
+		}
+		if fetched != 0 {
+			t.Errorf("%s should need no round trip, but the guard fetched %d time(s)", name, fetched)
+		}
+	}
+}
+
+// TestGuardFailsOpenOnAnUnreadableNotebook proves a transient read error does not
+// become a refusal to write. The server enforces the same rule, so letting the
+// write through costs a 422 at worst; refusing here would block a legal update
+// because an unrelated GET failed.
+func TestGuardFailsOpenOnAnUnreadableNotebook(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusInternalServerError)
+	}))
+	defer srv.Close()
+	if err := guardDefaultNotebookEncrypt(client.NewClient(srv.URL, "at_test"), "nb1", map[string]any{"default_encrypt": true}); err != nil {
+		t.Fatalf("an unreadable notebook should not block the write: %v", err)
+	}
+}
+
+// TestDefaultCannotEncryptServerMessageIsNotParaphrased pins that the server's
+// 422 reaches the user in the server's own words.
+//
+// The message is user-facing copy shared with the web app, and the issue asks for
+// it verbatim. mapNotebookError paraphrases the other notebook codes, so the risk
+// is somebody "helpfully" adding a case for this one; the CLI's default renderer
+// already prints an APIError's message, so the correct handling is to leave it
+// alone. This fails if a mapping is ever added.
+func TestDefaultCannotEncryptServerMessageIsNotParaphrased(t *testing.T) {
+	const serverMessage = "The default notebook can't encrypt notes by default — forwarded email, imports, and notes with no notebook land there; encrypt a different notebook instead."
+	apiError := &client.APIError{Code: "default_notebook_cannot_encrypt", Message: serverMessage, Status: 422}
+
+	got := mapNotebookError(apiError)
+	if got.Error() != apiError.Error() {
+		t.Fatalf("the server's message was rewritten locally:\n got: %s\nwant: %s", got.Error(), apiError.Error())
+	}
+	var still *client.APIError
+	if !errors.As(got, &still) {
+		t.Fatal("the APIError was replaced, so the renderer can no longer print its message and details")
+	}
+}
+
+// TestLocalRefusalMatchesTheServerWording keeps the refusal the CLI raises on its
+// own in step with the sentence the server would have sent. Two wordings for one
+// rule is how a user learns to distrust one of them.
+func TestLocalRefusalMatchesTheServerWording(t *testing.T) {
+	const serverMessage = "The default notebook can't encrypt notes by default — forwarded email, imports, and notes with no notebook land there; encrypt a different notebook instead."
+	// Lower only the FIRST character. Lowercasing the whole sentence would demand
+	// the CLI mangle a proper noun if the server copy ever gains one.
+	normalized := strings.TrimSuffix(strings.ToLower(serverMessage[:1])+serverMessage[1:], ".")
+	if defaultCannotEncryptMessage != normalized {
+		t.Errorf("the local refusal has drifted from the server's copy:\n local: %s\nserver: %s", defaultCannotEncryptMessage, normalized)
+	}
+}

cmd/skill.go

@@ -37,7 +37,7 @@ const (
 	// skillVersion is the content version of the bundled skill. Bump it whenever
 	// the skill files change; it is surfaced to the user on install and helps
 	// distinguish "already current" from "needs updating".
-	skillVersion = "1.1.1"
+	skillVersion = "1.1.2"
 
 	// codexBlockBegin / codexBlockEnd delimit the managed section the installer
 	// splices into a Codex AGENTS.md (a file the user may share with their own

cmd/sync.go

@@ -88,6 +88,7 @@ server_record you need to resolve it comes back in the results, so that stays 0.
 		if err != nil {
 			return err
 		}
+		warnDefaultNotebookEncrypt(changes)
 		data, err := c.SyncPush(map[string]any{"scope_id": scopeID, "device_id": deviceID, "changes": changes})
 		if err != nil {
 			return mapSyncError(err)
@@ -197,6 +198,43 @@ var syncAckCmd = &cobra.Command{
 // Helpers
 // ===========================================================================
 
+// warnDefaultNotebookEncrypt tells the user when a pushed notebook envelope would
+// make one notebook both the account default and encrypt-by-default.
+//
+// It WARNS rather than refuses, because that is what the server does here. Unlike
+// PATCH /notebooks/:id — which 422s and writes nothing — sync push coerces:
+// a pushed record that ends up default has default_encrypt forced to 0, and the
+// corrected record comes back on the next pull as an ordinary server-authoritative
+// overwrite. Refusing locally would break the passthrough contract of this command
+// (the envelopes are the user's own JSON) and reject a batch the server would have
+// accepted. Saying nothing would leave the user to discover on a later pull that a
+// flag they set had been quietly turned off.
+//
+// This is also the whole of the CLI's "sync engine cannot produce the pair"
+// obligation: the CLI keeps no local queue and constructs no notebook records of
+// its own — `sync push` forwards a JSON file the user wrote — so there is no
+// client-side state that could hold the banned pair. Pinned by
+// TestNoNotebookRecordsAreConstructedByTheCLI.
+func warnDefaultNotebookEncrypt(changes []any) {
+	for _, ch := range changes {
+		env, ok := ch.(map[string]any)
+		if !ok || str(env, "type") != "notebook" {
+			continue
+		}
+		rec := nested(env, "record")
+		if rec == nil {
+			continue
+		}
+		if boolean(rec, "is_default") && boolean(rec, "default_encrypt") {
+			fmt.Fprintln(os.Stderr, dim("Note: a pushed notebook is both the default and encrypt-by-default. The server\n"+
+				"cannot store that pair — the default is where forwarded email, imports and notes\n"+
+				"with no notebook land, and none of those can encrypt — so it will force\n"+
+				"default_encrypt off and send the corrected record back on your next pull."))
+			return
+		}
+	}
+}
+
 // runSyncPull pulls one chunk, or (when all is true) pages through every chunk
 // until caught up, merging them into one synthesized pull-response document.
 // Extracted from the command so the paging loop is unit-testable.