Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
cameronapak
left a comment
cameronapak
left a comment
There was a problem hiding this comment.
Review
Summary
Standards: 1 must-fix. Spec: 0 must-fix. Primary concern: the bookkeeping filter can silently remove consumer-facing release notes.
Review evidence
- Scope: reviewed revision, YPE-5753, PR discussion, the React SDK source PR, and the linked release-signoff thread.
- Method: traced Changesets input through parsing, filtering, deduplication, and root output; regenerated twice; exercised targeted synthetic notes; and tested a no-commit merge with current
main.
| Behavior or check | Method / command | Result | Evidence source |
|---|---|---|---|
| Historical completeness | Compared both package histories with generated output | All 10 versions and the hand-written 0.9.1 sections are present | Coordinator |
| Idempotence | Ran node scripts/build-root-changelog.mjs twice and compared hashes and Git diff |
Byte-identical, no diff | Coordinator |
| Bookkeeping boundaries | Added consumer prose beginning Updated dependencies and a third-party scoped package continuation |
Both were silently removed or truncated | Coordinator |
| Current-main compatibility | No-commit merge with origin/main, then regenerated |
Clean merge and unchanged root changelog | Coordinator |
- Limits: local Prettier execution was unavailable because dependencies were not installed in the orb. No live release was run.
- CI and bot review: all current checks pass; Greptile's earlier 0.9.1 finding is resolved.
- Event: REQUEST_CHANGES.
Written by Code Reviewer bot on behalf of Cam.
| * their own entry or trailing a real note as continuation lines. | ||
| */ | ||
| function stripBookkeeping(text) { | ||
| if (/^- Updated dependencies/.test(text)) return null |
There was a problem hiding this comment.
issue: Narrow this filter so it removes only Changesets bookkeeping, not consumer-facing release notes.
For Agents: preserve legitimate dependency notes
The current prefixes are broader than the fixed-group bookkeeping they describe. In a focused fixture,
- Updated dependencies to address CVE-1234.disappeared entirely, and an indented- @vendor/client@2.0.0line was removed even though that package is outside the fixed group. Match Changesets' generatedUpdated dependencies [hash]form and restrict bare package bumps to the configured fixed-group names. Add a regression fixture for both negative cases.
Written by Code Reviewer bot on behalf of Cam.
There was a problem hiding this comment.
If this is a nothing-burger, then please feel free to skip this
There was a problem hiding this comment.
Real, thanks. Both your fixtures failed before this.
Updated dependencies now requires the form with the commit hash, - Updated dependencies [80d3718]. Every occurrence in both changelogs carries one, so nothing legitimate stops matching. The bump filter is built from the fixed group names instead of any scoped package. Regenerated the root changelog and it is byte-identical.
Fixtures for both negatives are in scripts/build-root-changelog.test.mjs. I had to export the function and guard the main write to import it, and wire pnpm test:scripts into CI since root scripts/*.test.mjs was not running anywhere. That picks up the orphaned generate-locale-index.test.mjs too.
| .filter((d) => existsSync(join(PACKAGES, d, 'package.json'))) | ||
| .map((d) => [JSON.parse(readFileSync(join(PACKAGES, d, 'package.json'), 'utf8')).name, d]), | ||
| ) | ||
| const packages = fixedGroups[0].map((name) => { |
There was a problem hiding this comment.
praise: Taking the package list from the fixed group keeps unrelated package changelogs from silently changing the generated history.
For Agents: authoritative package inputs
This preserves the key safety property from the React SDK implementation: package membership comes from
.changeset/config.json, and a missing expected changelog fails before the root file is written.
Written by Code Reviewer bot on behalf of Cam.
The parser only recognised `### <level> Changes`, the form Changesets emits. React's changelogs are entirely Changesets output so that held there, but this repo's 0.9.1 was written by hand under `### Added` and `### Package surface`, and prose sat directly under the version heading. Those entries never set a kind and were dropped, so 0.9.1 rendered empty. Any `###` heading is now kept, verbatim when it is not a Changesets level, and ordered after the levels. Prose above the first heading is carried over with the same package annotation as entries. All 49 entries and both release summaries now survive. Found by Greptile.
0591a8a to
33b1b4d
Compare
|
Rebased onto current One thing for you to call: Suite is 23 passing after the rebase, and the root changelog regenerates byte-identically. |
cameronapak
left a comment
cameronapak
left a comment
There was a problem hiding this comment.
Review
Summary
Standards: 0 must-fix. Spec: 1 must-fix. Primary concern: the narrowed bookkeeping filter can still silently remove consumer-facing release notes that begin with a generated-looking hash.
Review evidence
- Scope: reviewed revision, the supplied YPE-5753 context, PR discussion, prior review, current fixes, and the full PR diff after its rebase onto current
main. - Method: traced package changelogs through parsing, filtering, deduplication, and rendering; regenerated twice; ran script tests and Prettier; and exercised focused negative filter cases.
| Behavior or check | Method / command | Result | Evidence source |
|---|---|---|---|
| Historical completeness | Compared package and root version headings and hand-written 0.9.1 sections | All 10 versions, both summaries, and all hand-written sections survive | Coordinator |
| Idempotence | Generated twice and compared SHA-256 hashes and Git diff | Byte-identical, no diff | Coordinator |
| Script regression tests | node --test scripts/*.test.mjs |
23 passed, 0 failed | Coordinator |
| Formatting | npx --yes prettier@3.8.3 --check ... and git diff --check |
Clean | Coordinator |
| Remaining filter boundary | Passed hash-bearing consumer prose and an outside-group continuation to stripBookkeeping |
Both returned null |
Coordinator |
- Limits: Kody/Jira was unavailable, so ticket intent came from Cam's supplied Jira text. No live release was run.
- CI and bot review: all current checks pass; earlier hand-written heading findings are resolved. Greptile separately noted that the rebased CI now runs the iOS script tests twice; that is non-blocking and not duplicated here.
- Event: REQUEST_CHANGES.
Written by Code Reviewer bot on behalf of Cam.
| * hand-written note that happens to open the same way, such as | ||
| * `- Updated dependencies to address CVE-1234.` | ||
| */ | ||
| const UPDATED_DEPENDENCIES = /^- Updated dependencies \[[0-9a-f]+\]/ |
There was a problem hiding this comment.
issue: Only discard an entry when the whole entry is fixed-group bookkeeping.
For Agents: finish narrowing the bookkeeping boundary
This regex still matches a prefix rather than the complete generated header. I reproduced
stripBookkeeping('- Updated dependencies [deadbee] to address CVE-1234.') === null; a block beginning with the same header and continuing with@vendor/client@2.0.0is also discarded. Require the header to occupy its full line, verify that any continuation lines are only fixed-group bumps, and add both negative cases beside the existing regression fixtures.
Written by Code Reviewer bot on behalf of Cam.
There was a problem hiding this comment.
Right, and the fix I shipped was the same mistake one level deeper. Reproduced both of yours before changing anything.
stripBookkeeping no longer decides from the first line. It drops the bookkeeping lines and judges the entry by what survives, so the header has to occupy its whole line (anchored both ends) and a surviving third-party bump keeps the entry alive. Removing the header can leave a continuation line first, so that gets promoted back to a top-level bullet and the renderer can still scope it.
Two fixtures added beside the existing ones, one for the CVE-style note and one for @vendor/client@2.0.0 under the header. Mutation-checked: restoring the prefix match or dropping the promotion each turns the suite red. 25 passing, and the root changelog still regenerates byte-identically.
|
|
||
| assert.deepEqual( | ||
| groups.map((g) => g.text), | ||
| ['**Scripture display**', '- `BibleTextView` renders a verse'], |
There was a problem hiding this comment.
praise: Pinning the expected subheading beside its first entry protects the exact hand-written history that the initial parser lost.
For Agents: historical section coverage
This assertion covers the heading-before-first-bullet boundary, while the adjacent test preserves the existing heading-after-bullet behavior. Together they catch the real 0.9.1 failure without weakening the normal Changesets path.
Written by Code Reviewer bot on behalf of Cam.
cameronapak
left a comment
cameronapak
left a comment
There was a problem hiding this comment.
Review
Summary
Standards: 0 must-fix. Spec: 0 must-fix. Primary concern: none.
Review evidence
- Scope: reviewed revision, the supplied YPE-5753 context, full PR behavior, prior review findings, Kyle's latest fix, and the rebase-related CI cleanup.
- Method: traced filtering through parsing, deduplication, package annotation, and rendering; exercised pure and mixed bookkeeping blocks; regenerated twice; and ran script and formatting checks.
| Behavior or check | Method / command | Result | Evidence source |
|---|---|---|---|
| Prior hash-bearing prose failure | Called stripBookkeeping with the reviewed CVE-style note |
Preserved unchanged | Coordinator |
| Prior outside-group continuation failure | Called stripBookkeeping with a generated header and third-party bump |
Preserved and promoted for package annotation | Coordinator |
| Fixed-group bookkeeping | Exercised pure and mixed dependency blocks | Fixed-group lines removed; consumer content retained | Coordinator |
| Historical completeness | Compared package and root histories | All 10 versions and package scopes remain present | Coordinator |
| Script regression tests | node --test scripts/*.test.mjs |
25 passed, 0 failed | Coordinator |
| Idempotence and formatting | Generated twice, compared SHA-256 hashes and Git diff, then ran Prettier and git diff --check |
Byte-identical and clean | Coordinator |
- Limits: Kody/Jira was unavailable, so ticket intent came from Cam's supplied Jira text. No live release was run.
- CI and bot review: all required GitHub checks pass. Greptile was still running at submission time; independent source and runtime verification was complete.
- Event: APPROVE.
Written by Code Reviewer bot on behalf of Cam.
| // package outside the fixed group. | ||
| const kept = text | ||
| .split('\n') | ||
| .filter((line) => !UPDATED_DEPENDENCIES.test(line) && !DEPENDENCY_BUMP.test(line)) |
There was a problem hiding this comment.
praise: Filtering known bookkeeping lines before judging the entry preserves consumer content without weakening the fixed-group cleanup.
For Agents: correct filtering boundary
This resolves both prior data-loss cases at their shared cause: an entry survives whenever meaningful content remains. The adjacent promotion step also restores the top-level bullet shape that the renderer needs for package attribution.
Written by Code Reviewer bot on behalf of Cam.
# Conflicts: # .github/workflows/ci.yml # package.json
2 participants
Closes YPE-5753, the RN Expo half of the work #389 did for the React SDK.
Swift and Kotlin each ship a root
CHANGELOG.md; this repo had only per-package files, so there was no single place to see what shipped in a release. This adds one, generated rather than maintained by hand.Why generated, and why deduped
coreanduiare afixedgroup, so they always share a version, and a changeset touching both writes the same entry into each changelog. Concatenating them would double most entries.Each entry appears once, annotated with which packages it affected: 16 ui-only, 8 all-packages, 3 core-only. That spread is why the annotation is worth having rather than printing "all packages" on every line.
Updated dependenciesblocks and bare@youversion/...@1.6.0bump lines are dropped. They are the fixed group's internal bookkeeping, not something a consumer reading release notes needs.Staying current
version-packagesbecomeschangeset version && node scripts/build-root-changelog.mjs.changesets/actionalready invokes it, so the Version Packages PR carries an up-to-date root changelog with no workflow change. Also exposed aspnpm build:root-changelog.Ported, not rewritten
scripts/build-root-changelog.mjscomes from the React SDK. It reads its package list from thefixedgroup in.changeset/config.json, so it adapted to two packages with no code change. Only prose differed. It imports node builtins only, so there is no new dependency.Verified
.prettierignoredoes not exclude*.md(React's did), so I checked rather than assumed: the generator's output is already Prettier-clean, andprettier --writeleaves it untouched.Note for whoever reviews both
This and #201 both touch
package.json, in different keys — #201 adds devDependencies andtest:ci-scripts, this changesversion-packages. They should merge cleanly; whichever lands second may want a trivial rebase.Worth knowing they interact by design: #201's
generated_releasejob verifies the release PR by re-runningversion-packagesand comparing complete git trees. This generator being idempotent is what keeps that reproducible.The PR appears safe to merge; no outstanding previous finding or actionable new issue remains.
Summary
The PR adds a root changelog generated from the fixed-group package changelogs and wires regeneration into
version-packages.Diagram
%%{init: {'theme': 'neutral'}}%% flowchart LR A[changeset version] --> B[Per-package changelogs] B --> C[Build root changelog] C --> D[Deduplicate and label entries] D --> E[CHANGELOG.md]Reviews (6) · Last reviewed commit: "Merge remote-tracking branch 'origin/mai..."