Fix #83: refuse the default-notebook/encrypt-by-default pair locally - #92
Merged
Conversation
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.
- 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.
…overclaiming it 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #83
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. The server enforces it with
422 default_notebook_cannot_encrypt; the CLI sent the request anyway and relayed the failure.What changed
guardDefaultNotebookEncryptrefuses both directions before the write — turning encryption on for the current default, and promoting a notebook that already encrypts.It judges the RESULTING state, exactly as the server does. This is the part worth getting right:
--make-default --default-encrypt=falselegally promotes a notebook while switching encryption off. An implementation that asked "does this request mention both flags?" would block the single command that fixes an encrypting notebook someone wants as their default. Verified below.It reads the notebook only when the answer depends on current state. A request stating both fields is decided with no round trip, and so is anything that turns encryption off, or a plain rename. An unreadable notebook fails open: the server enforces the same rule, so a transient GET failure costs a 422 at worst — turning it into a local refusal would block a legal update because an unrelated read failed.
default_notebook_cannot_encryptis deliberately NOT added tomapNotebookError. Its message is user-facing copy shared with the web app, and the CLI's default renderer already prints anAPIError's message verbatim — so paraphrasing it here is the one way to get it wrong. A test pins that it is never mapped, and a second keeps the CLI's own local refusal in step with the server's sentence.sync pushwarns rather than refuses, because the server behaves differently there:PATCH /notebooks/:id422s and writes nothing, butsync pushcoerces — the record lands withdefault_encryptforced off and comes back corrected on the next pull. Refusing locally would reject a batch the server would have accepted; silence would let a flag the user set disappear without explanation.The "sync engine cannot produce the pair" requirement holds by construction. The CLI keeps no offline queue, and there are exactly two
SyncPushcall sites — the push command, which forwards a JSON file the user wrote, and the crypto keystore. So no client-side state can hold a default notebook withdefault_encrypton.TestNoNotebookRecordsAreConstructedByTheCLIis a canary over that property, 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, including in a package that does not exist yet — but a determined one slips past: a struct with a json tag, a const indirection, or a value assembled at runtime. A failure is certain; a pass means "nothing obvious". The test says so in its own comment.Verification
End-to-end against a real server, throwaway account, isolated
HOME. All seven checks passed.Both directions refused:
The legal both-fields case still works — the check a naive implementation fails:
The refusal is local, not a relayed 422. Pointing the CLI at a request-logging server shows exactly what goes on the wire:
To be precise: the banned command is not zero-traffic — it issues one read-only GET to learn whether the target is currently the default. No write is sent and no USN is spent.
The server really would have 422'd (the guard enforces a real rule, not an invented one):
The CLI's sentence is character-for-character the server's, em dash and semicolon included, with the leading
Thelowercased and the trailing period dropped per Go error convention.sync pushcoerces, and the CLI says so:Edge cases also checked:
--make-default --default-encrypt(both explicitly true) is refused;--default-encrypt=falseon the default notebook succeeds as a harmless no-op.Found while testing — a server bug, filed separately
When a pushed sync envelope would move the default (promote notebook B while A still holds it), the server rejects with a raw SQLite string rather than demoting the prior default the way
PATCH /notebooks/:iddoes:The CLI handled it correctly — warned, surfaced the rejection, exit 1. Unrelated to this change; filed as HarborMyNotes/app.harbor.my#1388.