From 42cc767a23e6d104496913ca539005a77b9cc3cd Mon Sep 17 00:00:00 2001 From: Spicer Matthews Date: Thu, 6 Aug 2026 20:49:10 -0700 Subject: [PATCH 1/3] Fix #83: refuse the default-notebook/encrypt-by-default pair locally MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A notebook can never be both the account default and encrypt-by-default: the default is where forwarded email, imports and notes with no notebook land, and none of those writers can client-side encrypt. The server enforces it (422 default_notebook_cannot_encrypt); the CLI sent the request anyway and relayed the failure. - guardDefaultNotebookEncrypt refuses both directions before the write: turning encryption on for the current default, and promoting a notebook that already encrypts. Judged on the RESULTING state, like the server — '--make-default --default-encrypt=false' legally promotes while switching encryption off, and blocking that would break the one command that fixes the situation. - It reads the notebook only when the answer depends on current state. A request stating both fields needs no round trip, nor does anything that turns encryption off. An unreadable notebook fails OPEN: the server enforces the same rule, so a transient GET failure must not become a refusal to write. - default_notebook_cannot_encrypt is deliberately left out of mapNotebookError. Its message is user-facing copy shared with the web app and the default renderer prints it verbatim; a test pins that it is never paraphrased, and another keeps the local refusal's wording in step with the server's. - sync push warns rather than refuses, because the server COERCES there instead of rejecting. The CLI keeps no offline queue and builds no notebook records, so it cannot produce the pair by construction — a test pins that structurally rather than by assertion. - Help text on both flags and in the update long-form names the rule and shows the escape hatch. --- cmd/notebooks.go | 85 ++++++++++++++++++++++-- cmd/notebooks_test.go | 150 ++++++++++++++++++++++++++++++++++++++++++ cmd/sync.go | 38 +++++++++++ cmd/sync_test.go | 84 +++++++++++++++++++++++ 4 files changed, 352 insertions(+), 5 deletions(-) diff --git a/cmd/notebooks.go b/cmd/notebooks.go index 5127112..bf491db 100644 --- a/cmd/notebooks.go +++ b/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) @@ -166,11 +177,75 @@ func mapNotebookError(err error) error { return errors.New("the default notebook cannot be deleted — promote another notebook first") case "cannot_unset_default": return errors.New("there must always be a default notebook — promote a different one instead") + // default_notebook_cannot_encrypt is deliberately NOT mapped. Its server + // message is user-facing copy shared with the web app, and the default + // renderer prints an APIError's message verbatim — so paraphrasing it + // here is the one way to get it wrong. Pinned by + // TestDefaultCannotEncryptServerMessageIsNotParaphrased. } } 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 without a round trip, and so the failure reads the same offline as on. +// 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 // =========================================================================== @@ -235,13 +310,13 @@ func init() { notebooksCreateCmd.Flags().String("name", "", "Notebook name (required)") notebooksCreateCmd.Flags().String("stack", "", "Stack (grouping label)") - notebooksCreateCmd.Flags().Bool("default-encrypt", false, "Encrypt new notes in this notebook by default") + notebooksCreateCmd.Flags().Bool("default-encrypt", false, "Encrypt new notes in this notebook by default (never allowed on the default notebook)") 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") diff --git a/cmd/notebooks_test.go b/cmd/notebooks_test.go index b59fbc3..6ec14d5 100644 --- a/cmd/notebooks_test.go +++ b/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,147 @@ 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." + normalized := strings.TrimSuffix(strings.ToLower(serverMessage), ".") + if defaultCannotEncryptMessage != normalized { + t.Errorf("the local refusal has drifted from the server's copy:\n local: %s\nserver: %s", defaultCannotEncryptMessage, normalized) + } +} diff --git a/cmd/sync.go b/cmd/sync.go index b45ed43..0e5f147 100644 --- a/cmd/sync.go +++ b/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) @@ -559,3 +560,40 @@ func init() { syncCmd.AddCommand(syncPullCmd, syncPushCmd, syncDevicesCmd, syncRegisterDeviceCmd, syncRemoveDeviceCmd, syncAckCmd) rootCmd.AddCommand(syncCmd) } + +// 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 + } + } +} diff --git a/cmd/sync_test.go b/cmd/sync_test.go index ab39d68..5e24cca 100644 --- a/cmd/sync_test.go +++ b/cmd/sync_test.go @@ -10,6 +10,7 @@ import ( "net/http" "net/http/httptest" "os" + "path/filepath" "strings" "testing" @@ -263,3 +264,86 @@ func TestSyncPushCommandExitsZeroWhenEverythingApplied(t *testing.T) { t.Fatalf("a clean push must exit 0: %v", err) } } + +// TestWarnDefaultNotebookEncrypt proves the push path says something when a +// pushed notebook record carries the banned pair, and stays quiet otherwise. +// +// It warns rather than refuses because sync push COERCES server-side (unlike +// PATCH /notebooks/:id, which 422s): the record lands with default_encrypt +// forced off and comes back corrected on the next pull. Refusing would reject a +// batch the server would have accepted; silence would let a flag the user set +// disappear without explanation. +func TestWarnDefaultNotebookEncrypt(t *testing.T) { + banned := []any{map[string]any{ + "type": "notebook", "id": "nb1", + "record": map[string]any{"id": "nb1", "is_default": true, "default_encrypt": true}, + }} + out := captureStderr(t, func() { warnDefaultNotebookEncrypt(banned) }) + for _, want := range []string{"default", "force", "next pull"} { + if !strings.Contains(out, want) { + t.Errorf("the warning does not mention %q:\n%s", want, out) + } + } + + quiet := []any{ + map[string]any{"type": "notebook", "id": "nb2", + "record": map[string]any{"id": "nb2", "is_default": true, "default_encrypt": false}}, + map[string]any{"type": "notebook", "id": "nb3", + "record": map[string]any{"id": "nb3", "is_default": false, "default_encrypt": true}}, + map[string]any{"type": "note", "id": "n1", + "record": map[string]any{"id": "n1", "is_default": true, "default_encrypt": true}}, + map[string]any{"type": "notebook", "id": "nb4"}, // no record at all + "not an envelope", + } + if out := captureStderr(t, func() { warnDefaultNotebookEncrypt(quiet) }); out != "" { + t.Errorf("warned about a legal push:\n%s", out) + } +} + +// TestWarnDefaultNotebookEncryptSpeaksOnce proves a batch full of offending +// notebooks produces one warning, not one per record — a push is a batch, and a +// wall of identical warnings is how people learn to scroll past them. +func TestWarnDefaultNotebookEncryptSpeaksOnce(t *testing.T) { + var changes []any + for i := 0; i < 5; i++ { + changes = append(changes, map[string]any{ + "type": "notebook", "id": "nb", + "record": map[string]any{"id": "nb", "is_default": true, "default_encrypt": true}, + }) + } + out := captureStderr(t, func() { warnDefaultNotebookEncrypt(changes) }) + if n := strings.Count(out, "cannot store that pair"); n != 1 { + t.Errorf("warned %d times for one batch, want 1:\n%s", n, out) + } +} + +// TestNoNotebookRecordsAreConstructedByTheCLI is the CLI's whole "the sync engine +// cannot produce the banned pair" obligation, discharged structurally. +// +// The CLI keeps no offline queue: `sync push` forwards a JSON file the user +// wrote, and the only sync record the CLI builds itself is the crypto keystore. +// So there is no client-side state that could hold a default notebook with +// default_encrypt on. This test fails the day that stops being true — if someone +// adds code that constructs a "notebook" sync record, the guarantee needs +// rethinking rather than silently lapsing. +func TestNoNotebookRecordsAreConstructedByTheCLI(t *testing.T) { + files, err := filepath.Glob("*.go") + if err != nil { + t.Fatal(err) + } + for _, f := range files { + if strings.HasSuffix(f, "_test.go") { + continue + } + src, err := os.ReadFile(f) + if err != nil { + t.Fatal(err) + } + // The keystore record in crypto.go is the one the CLI legitimately builds. + for _, banned := range []string{`"type": "notebook"`, `"type": "notebook"`} { + if strings.Contains(string(src), banned) { + t.Errorf("%s constructs a notebook sync record — the CLI's 'no local state can hold the banned pair' guarantee no longer holds by construction", f) + } + } + } +} From 65a066948db94dcb51b27fbb8083b3bd7ec36400 Mon Sep 17 00:00:00 2001 From: Spicer Matthews Date: Thu, 6 Aug 2026 21:20:51 -0700 Subject: [PATCH 2/3] Address review: harden the structural test, document the rule for agents MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - TestNoNotebookRecordsAreConstructedByTheCLI matched two exact spacings in one package, so it passed on the most natural envelope shape gofmt produces. Use a regex across cmd/, client/, crypto/ and config/, with a floor on files scanned. Verified it now catches what it missed. - The bundled agent skill documented --make-default and --default-encrypt without the ban between them; that file is what 'harbor skill install' ships to Claude/Codex/Cursor, so an agent read the incomplete rule. - Move the 'deliberately not mapped' comment above the switch — inside the last case it read as belonging to cannot_unset_default. - Drop the '(never allowed on the default notebook)' note from the CREATE flag: a notebook being created is never the default, so it was a non-sequitur there. It stays on update, where it applies. --- cmd/assets/skill/reference.md | 11 +++++++++++ cmd/notebooks.go | 12 ++++++------ cmd/sync_test.go | 35 +++++++++++++++++++++++------------ 3 files changed, 40 insertions(+), 18 deletions(-) diff --git a/cmd/assets/skill/reference.md b/cmd/assets/skill/reference.md index 3078279..e82796c 100644 --- a/cmd/assets/skill/reference.md +++ b/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 --make-default --default-encrypt=false +``` + --- ## Notes (aliases: `note`, `n`) diff --git a/cmd/notebooks.go b/cmd/notebooks.go index bf491db..46f2475 100644 --- a/cmd/notebooks.go +++ b/cmd/notebooks.go @@ -167,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) { @@ -177,11 +182,6 @@ func mapNotebookError(err error) error { return errors.New("the default notebook cannot be deleted — promote another notebook first") case "cannot_unset_default": return errors.New("there must always be a default notebook — promote a different one instead") - // default_notebook_cannot_encrypt is deliberately NOT mapped. Its server - // message is user-facing copy shared with the web app, and the default - // renderer prints an APIError's message verbatim — so paraphrasing it - // here is the one way to get it wrong. Pinned by - // TestDefaultCannotEncryptServerMessageIsNotParaphrased. } } return err @@ -310,7 +310,7 @@ func init() { notebooksCreateCmd.Flags().String("name", "", "Notebook name (required)") notebooksCreateCmd.Flags().String("stack", "", "Stack (grouping label)") - notebooksCreateCmd.Flags().Bool("default-encrypt", false, "Encrypt new notes in this notebook by default (never allowed on the default notebook)") + notebooksCreateCmd.Flags().Bool("default-encrypt", false, "Encrypt new notes in this notebook by default") notebooksUpdateCmd.Flags().String("name", "", "New name") notebooksUpdateCmd.Flags().String("stack", "", "New stack") diff --git a/cmd/sync_test.go b/cmd/sync_test.go index 5e24cca..628a1be 100644 --- a/cmd/sync_test.go +++ b/cmd/sync_test.go @@ -11,6 +11,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "regexp" "strings" "testing" @@ -327,23 +328,33 @@ func TestWarnDefaultNotebookEncryptSpeaksOnce(t *testing.T) { // adds code that constructs a "notebook" sync record, the guarantee needs // rethinking rather than silently lapsing. func TestNoNotebookRecordsAreConstructedByTheCLI(t *testing.T) { - files, err := filepath.Glob("*.go") - if err != nil { - t.Fatal(err) - } - for _, f := range files { - if strings.HasSuffix(f, "_test.go") { - continue - } - src, err := os.ReadFile(f) + // A regex, not a fixed string: gofmt aligns struct-literal keys to the longest + // one, so the spacing after "type" changes with the other keys in the map. An + // exact-match check passes on the most natural envelope shape and is worthless. + banned := regexp.MustCompile(`"type"\s*:\s*"notebook"`) + + roots := []string{".", "../client", "../crypto", "../config"} + scanned := 0 + for _, root := range roots { + files, err := filepath.Glob(filepath.Join(root, "*.go")) if err != nil { t.Fatal(err) } - // The keystore record in crypto.go is the one the CLI legitimately builds. - for _, banned := range []string{`"type": "notebook"`, `"type": "notebook"`} { - if strings.Contains(string(src), banned) { + for _, f := range files { + if strings.HasSuffix(f, "_test.go") { + continue + } + src, err := os.ReadFile(f) + if err != nil { + t.Fatal(err) + } + scanned++ + if banned.Match(src) { t.Errorf("%s constructs a notebook sync record — the CLI's 'no local state can hold the banned pair' guarantee no longer holds by construction", f) } } } + if scanned < 20 { + t.Fatalf("only scanned %d files — the walk is not reaching the source tree", scanned) + } } From 1cd347ac273bd4827d19ee40a6a3ef2bc8558acb Mon Sep 17 00:00:00 2001 From: Spicer Matthews Date: Thu, 6 Aug 2026 21:39:49 -0700 Subject: [PATCH 3/3] Address review: walk the whole module in the structural canary, stop overclaiming it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reviewer wrote three regressions the test waved through — a struct with a json tag, a const indirection, and a notebook record in a NEW package, which the hardcoded root list could not see at all. The last one is now caught: walk the module from its root instead of naming four directories, with a floor on files scanned. The other two still slip past, and a grep never will catch them, so the test and the PR now say plainly that it is a canary over a property that holds by construction — not a proof of it. A failure is certain; a pass means 'nothing obvious'. - The doc comment claimed the refusal 'reads the same offline as on'. Only true for a request stating both fields; the single-flag cases read the notebook first, so offline they surface the read error instead. - The wording test lowercased the whole server sentence, which would demand the CLI mangle a proper noun if the copy ever gains one. Lower the first character only. - Bump skillVersion, whose own comment says to do so when the bundled skill files change. - Move warnDefaultNotebookEncrypt into the file's Helpers section. --- cmd/notebooks.go | 6 ++-- cmd/notebooks_test.go | 4 ++- cmd/skill.go | 2 +- cmd/sync.go | 74 +++++++++++++++++++++---------------------- cmd/sync_test.go | 66 +++++++++++++++++++++++--------------- 5 files changed, 86 insertions(+), 66 deletions(-) diff --git a/cmd/notebooks.go b/cmd/notebooks.go index 46f2475..b250d95 100644 --- a/cmd/notebooks.go +++ b/cmd/notebooks.go @@ -195,8 +195,10 @@ func mapNotebookError(err error) error { // 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 without a round trip, and so the failure reads the same offline as on. +// 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" diff --git a/cmd/notebooks_test.go b/cmd/notebooks_test.go index 6ec14d5..91fd6b8 100644 --- a/cmd/notebooks_test.go +++ b/cmd/notebooks_test.go @@ -191,7 +191,9 @@ func TestDefaultCannotEncryptServerMessageIsNotParaphrased(t *testing.T) { // 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." - normalized := strings.TrimSuffix(strings.ToLower(serverMessage), ".") + // 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) } diff --git a/cmd/skill.go b/cmd/skill.go index 34b9144..3b61ddf 100644 --- a/cmd/skill.go +++ b/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 diff --git a/cmd/sync.go b/cmd/sync.go index 0e5f147..bb6945a 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -198,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. @@ -560,40 +597,3 @@ func init() { syncCmd.AddCommand(syncPullCmd, syncPushCmd, syncDevicesCmd, syncRegisterDeviceCmd, syncRemoveDeviceCmd, syncAckCmd) rootCmd.AddCommand(syncCmd) } - -// 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 - } - } -} diff --git a/cmd/sync_test.go b/cmd/sync_test.go index 628a1be..63c873f 100644 --- a/cmd/sync_test.go +++ b/cmd/sync_test.go @@ -7,6 +7,7 @@ import ( "encoding/json" "errors" "io" + "io/fs" "net/http" "net/http/httptest" "os" @@ -318,43 +319,58 @@ func TestWarnDefaultNotebookEncryptSpeaksOnce(t *testing.T) { } } -// TestNoNotebookRecordsAreConstructedByTheCLI is the CLI's whole "the sync engine -// cannot produce the banned pair" obligation, discharged structurally. +// TestNoNotebookRecordsAreConstructedByTheCLI is the canary over the CLI's "the +// sync engine cannot produce the banned pair" property. // -// The CLI keeps no offline queue: `sync push` forwards a JSON file the user -// wrote, and the only sync record the CLI builds itself is the crypto keystore. -// So there is no client-side state that could hold a default notebook with -// default_encrypt on. This test fails the day that stops being true — if someone -// adds code that constructs a "notebook" sync record, the guarantee needs -// rethinking rather than silently lapsing. +// The property holds by construction: the CLI keeps no offline queue, and there +// are exactly two SyncPush call sites — this file's push command, which forwards +// a JSON file the USER wrote, and cmd/crypto.go, which pushes the keystore. So +// there is no client-side state that could hold a default notebook with +// default_encrypt on. +// +// This test is a canary, NOT a proof. It walks the whole module and greps for a +// notebook type tag, which catches the obvious regression — someone building a +// notebook envelope inline — but a determined one slips past: a struct with a +// json tag, a const indirection, or a value assembled at runtime. Treat a +// failure here as certain, and a pass as "nothing obvious", not as a guarantee. +// A real proof would parse with go/ast and follow the values into SyncPush. func TestNoNotebookRecordsAreConstructedByTheCLI(t *testing.T) { // A regex, not a fixed string: gofmt aligns struct-literal keys to the longest // one, so the spacing after "type" changes with the other keys in the map. An - // exact-match check passes on the most natural envelope shape and is worthless. + // exact-match check passes on the most natural envelope shape. banned := regexp.MustCompile(`"type"\s*:\s*"notebook"`) - roots := []string{".", "../client", "../crypto", "../config"} + // Walk the whole module from its root rather than a hardcoded package list, so + // a new package cannot be invisible to this simply by existing. scanned := 0 - for _, root := range roots { - files, err := filepath.Glob(filepath.Join(root, "*.go")) + err := filepath.WalkDir("..", func(path string, d fs.DirEntry, err error) error { if err != nil { - t.Fatal(err) + return err } - for _, f := range files { - if strings.HasSuffix(f, "_test.go") { - continue - } - src, err := os.ReadFile(f) - if err != nil { - t.Fatal(err) - } - scanned++ - if banned.Match(src) { - t.Errorf("%s constructs a notebook sync record — the CLI's 'no local state can hold the banned pair' guarantee no longer holds by construction", f) + if d.IsDir() { + switch d.Name() { + case ".git", "build", "dist", "vendor", "node_modules": + return filepath.SkipDir } + return nil + } + if !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { + return nil + } + src, err := os.ReadFile(path) + if err != nil { + return err + } + scanned++ + if banned.Match(src) { + t.Errorf("%s constructs a notebook sync record — the CLI's 'no local state can hold the banned pair' property no longer holds by construction", path) } + return nil + }) + if err != nil { + t.Fatal(err) } - if scanned < 20 { + if scanned < 40 { t.Fatalf("only scanned %d files — the walk is not reaching the source tree", scanned) } }