Developer guide: five more sentences that promise code - #5814
Merged
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 20594ac2ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba8032785e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea39639e0b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 269f91bc7b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Contributor
Cloudflare Preview
|
Contributor
|
Developer Guide build artifacts are available for download from this workflow run:
Developer Guide quality checks: |
Three get a compiled listing and two get one that cannot compile here, for the same reason each time -- the class is not on the docs module's classpath. Compiled: the custom Mapper<T>, which the JSON/XML chapter told the reader to hand-write for a type a third-party jar owns and then showed nothing. It implements all six methods, because the interface has six and an example missing writeXml is one a reader cannot follow. And the build-hint pair in Miscellaneous Features, guarded by isSimulator() -- setProjectBuildHint throws anywhere else, which the javadoc says and the chapter did not. Not compiled: the storage password migration, which drives EncryptedStorage from the bouncy castle cn1lib, and the SVG registry call, whose class only exists after a build has run the transcoder. Both are [listing] blocks with a sentence saying why. The migration also had a bug carried over from the original: it read storageFileName and wrote "TestEncryption", so it moved one entry into a different name. The fifth is rewritten instead. "There are two simple methods in the Util class:" was followed by two one-argument signatures; naming xorEncode and xorDecode in the sentence says the same thing without a listing. Ratchet drops from 34 to 29. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The listing decrypted one entry and re-encrypted it, and the key is installed for the whole of Storage -- so every other entry stayed under the old key and became unreadable the moment the new one went in. It was the shape the pre-extraction original had, and it would have cost somebody their data. It now reads them all, swaps the key, then writes them all back, and says what that costs: everything is held in memory between the two loops, which suits a handful of records and not a storage full of cached images. The SVG listing also named DENSITY_HIGH bare. The constant is on CN1Constants and inherited by Display, so neither the class import nor a wildcard brings it in -- Display.DENSITY_HIGH, with the import beside it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Util.cleanup swallows a close failure by design, and on the write side of a key migration that is the one place it must not: a swallowed close means an entry was never finalized under the new key, and the loop carries on and reports success having lost it. Both loops use try-with-resources now, with the reason beside them. And the build-hint listing dereferenced getProjectBuildHints() straight away. JavaSEPort returns null from it when codenameone_settings.properties is missing or unreadable -- isSimulator() being true is not enough -- so the example crashed in the one environment it was written for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The two-loop version overwrote entries in place under the new key. Killed part way through -- or failing on one write -- it left some entries under each key, and the plaintext it needed to recover went with the process. Neither key then reads the whole store. It is now staged. Phase one reads everything under the old key; phase two writes every entry to a "<name>.migrating" copy under the new one, leaving the originals untouched and still readable with the old key, so a crash anywhere in there costs nothing. A marker entry listing the names is the commit point, and finishMigration -- which also runs at startup -- replaces the originals from the staged copies and removes them. Every step of that is idempotent, so a crash during recovery only means recovery runs again, and it needs the new key alone. Storage has no rename, so this is as close to atomic as the API allows. The costs are stated where the listing ends: everything is in memory between the two phases, and Storage has to be quiet throughout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…a listing The staging protocol added last round answered one failure mode and opened two more: the marker that makes recovery possible is itself written after the vulnerable phase and under the new key, so a crash before it lands leaves a store nothing can recover, and the staging names live in the same namespace as the app's own entries. Both are real, and chasing them is the wrong direction. Storage has no rename, so the converted copies cannot be swapped in atomically no matter how the loop is arranged, and every further step makes a documentation listing longer without making it correct. Designing a crash-safe key rotation protocol inside this chapter was scope I should not have taken. So the listing goes back to the plain two-phase form, and the guide says what it does not guarantee: if the process dies part way through, some entries are under each key and neither opens the whole store. And it says what to do instead, which is the answer that actually removes the problem: encrypt Storage with a random key the app generates once, keep that key wrapped under the password-derived one in a single record outside Storage, and a password change re-wraps that one record. Nothing else moves, so there is no half-converted state to recover from. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
shai-almog
force-pushed
the
dg-code-blocks-2
branch
from
September 14, 2026 01:45
c1d62dc to
8696939
Compare
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
1 participant
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.
Five more holes in the
check-missing-code-blocksratchet, across four chapters. Three get a compiled listing, one gets a[listing]block, one is rewritten.Compiled
Annotation-JSON-XML-Mappingtold the reader to hand-write aMapper<T>for a type a third-party jar owns, and showed nothing. The new listing implements all six interface methods —type,toMap,fromMap,xmlRootName,writeXml,readXml— because an example missingwriteXmlis one a reader cannot actually follow. It also reads numbers throughNumberrather than casting toDouble, with the reason in a comment:JSONParserhands backDouble, a map from elsewhere may not, and a failed cast doesnt throw on iOS.Miscellaneous-Featurespromised the twoDisplaybuild-hint APIs. The listing is guarded byisSimulator()—setProjectBuildHintthrows anywhere else, which its javadoc says and the chapter didnt.Not compilable here, and said so
security— the storage password migration drivesEncryptedStorage, which lives in the bouncy castle cn1lib.SVG-Transcoder— the generated class only exists after a build has run the transcoder.Both are
[listing]blocks with a sentence explaining why theyre not among the compiled examples.The migration listing also had a bug carried over from the pre-extraction original: it read
storageFileNameand wrote"TestEncryption", so it moved one entry into a different name instead of re-encrypting it in place.Rewritten
"There are two simple methods in the
Utilclass:" was followed by two one-argument signatures. NamingxorEncodeandxorDecodein the sentence conveys the same thing without a listing.Gates
check-missing-code-blocksratchet 34 → 29validate-guide-snippets.py— 1112 include-backed blockscodenameone-core(theMapperone caught that the interface has six methods, not three)asciidoctor --failure-level WARNandasciidoctor-pdf— cleanstatus: ok,total: 0on all four chapters