Skip to content

Render review pages from structured commit messages - #8861

Closed
cristianoc wants to merge 1 commit into
masterfrom
cristianoc/commit-records
Closed

Render review pages from structured commit messages#8861
cristianoc wants to merge 1 commit into
masterfrom
cristianoc/commit-records

Conversation

@cristianoc

@cristianoc cristianoc commented Oct 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Render review pages from structured commit messages

Review threads in this repository keep asking the same questions:

This PR adds a commit message format, the commit record, that answers these questions in the commit itself, and a script that renders review pages from records.

  • .claude/skills/records/SKILL.md defines the format: prose sections (Example (rescript), Before (js) / After (js), Source, Breaking, Changelog entry, Review) and git trailers (Area, Changelog, Output, Formatting, Formats, PPX, Editor, Errors, Tests, Ran, Measured, Depends). Each field links to the PRs whose threads raised it. Coding agents load it as a skill; for people it is a plain document.
  • scripts/commitRecords.js renders one card per commit: the ReScript example beside the JavaScript before and after, the changelog category and entry, the effects, and tests by suite. It checks each record against its own diff: the changelog fragment, modified generated JavaScript, magic-number bumps, v0 PPX bridge fixtures, and ERROR_VARIANTS.md. For a range, it adds a summary, a changelog draft, and the files that more than one commit changes. It reads only git and has no dependencies.
  • AGENTS.md points agents to the format.
node scripts/commitRecords.js --out review.html master..my-branch

Demo: #8862, stacked on this PR, is a small codegen change whose commit message is a record. The rendered page for both commits is at https://claude.ai/artifact/6nbY5DMccWCk7yYDbuoZrH. To render it locally:

node scripts/commitRecords.js --out review.html master..cristianoc/int-identity-ops

Everything is opt-in. Nothing in CI enforces the format, and a commit without a record still renders, with its card flagging the missing Changelog trailer. Feedback on which fields are worth keeping is the main question here.

Test plan

  • node --test scripts/commitRecords.test.js: 2 passing. The tests render records from a temporary repository and check the cards and the diff checks.
  • npx biome check on both scripts: no findings. npx tsc: exit 0.

🤖 Generated with Claude Code

cristianoc added a commit that referenced this pull request Oct 14, 2026
@cristianoc @claude
Compile integer operations with an identity operand to the operand
faf5ecb 
Integer `x + 0`, `0 + x`, `x - 0`, `x * 1`, `1 * x` and `x / 1` compile
to `x`. A ReScript `int` value is a 32-bit integer in JavaScript, so the
`| 0` truncation (or `<< 0` for `* 1`) that these operations emitted
leaves the value unchanged. The JavaScript expression builders in
`Js_exp_make` (`int32_add`, `int32_minus`, `int32_mul`, `int32_div`)
return the other operand when one operand is the literal identity. The
dropped operand is a literal, so evaluation of the kept operand is
unchanged.

Example (rescript):
    let addZero = (x: int) => x + 0
    let subZero = (x: int) => x - 0
    let mulOne = (x: int) => x * 1
    let divOne = (x: int) => x / 1

Before (js):
    function addZero(x) {
      return x + 0 | 0;
    }

    function subZero(x) {
      return x - 0 | 0;
    }

    function mulOne(x) {
      return (x << 0);
    }

    function divOne(x) {
      return x / 1 | 0;
    }

After (js):
    function addZero(x) {
      return x;
    }

    function subZero(x) {
      return x;
    }

    function mulOne(x) {
      return x;
    }

    function divOne(x) {
      return x;
    }

Source: `bsc Example.res` (CommonJS, to stdout) with bsc built from
aa2078e (Before) and from this commit (After); function bodies shown, the
module header and exports are identical.

Changelog entry: - Compile integer `x + 0`, `0 + x`, `x - 0`, `x * 1`,
    `1 * x` and `x / 1` to `x`. #8862

Review: Can an `int` operand hold a value outside the 32-bit range? Every
    int the compiler computes is already truncated: compiling `-x`,
    `Int.fromFloat(f)` and `Int.shiftRightUnsigned(x, y)` with this commit
    gives `-x | 0`, `f | 0` and `(x >>> y) | 0`. A value outside the range
    reaches an `int` only through an external or `%raw` typed `int`; for
    those the result changes, for example `Number.MAX_SAFE_INTEGER + 0` gave
    -1 and gives 9007199254740991. The compiler already relies on the same
    assumption: `Plsrint` by the literal 0 compiles to the operand
    (compiler/core/lam_compile_primitive.ml, `[e1; 0] -> e1`), dropping
    `(x >>> 0) | 0`.

Review: Does `x / 1` still check for division by zero? The divisor is the
    literal 1, so no check applies; the existing rule for a nonzero literal
    divisor already skipped the runtime `div` call.

Area: codegen tests
Changelog: polish
Output: changed integer `+ 0`, `0 +`, `- 0`, `* 1`, `1 *`, `/ 1` on a
 non-literal operand, now the bare operand; in tests/tests/src one existing
 snapshot changes (gpr_3609_test.mjs, `0 + state._0 | 0` -> `state._0`),
 and the stdlib build (make lib) emits no changed JS
Formats: none
Tests: +tests/tests/src/int_identity_ops_test.res
 +tests/tests/src/int_identity_ops_test.mjs
 ~tests/tests/src/gpr_3609_test.mjs
Ran: make (dune exit 0); make lib (exit 0); make test (exit 0: 269 ounit
 tests OK, tests/tests mocha 606 passing including Int_identity_ops_test,
 stdlib mocha 744 passing); make checkformat (exit 0); yarn changelog:check
 (fails only on the XXXX placeholder link, as expected before a PR exists)
Depends: #8861 (stacked)
PR: #8862
Signed-off-by: Cristiano Calcagno <ccrisccris@gmail.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cristianoc
cristianoc force-pushed the cristianoc/commit-records branch from 47ae711 to 9e5f07c Compare October 14, 2026 07:13
@cristianoc
cristianoc added this pull request to stack #8863 October 14, 2026 07:13
cristianoc added a commit that referenced this pull request Oct 14, 2026
@cristianoc @claude
Compile integer operations with an identity operand to the operand
a253891 
Integer `x + 0`, `0 + x`, `x - 0`, `x * 1`, `1 * x` and `x / 1` compile
to `x`. A ReScript `int` value is a 32-bit integer in JavaScript, so the
`| 0` truncation (or `<< 0` for `* 1`) that these operations emitted
leaves the value unchanged. The JavaScript expression builders in
`Js_exp_make` (`int32_add`, `int32_minus`, `int32_mul`, `int32_div`)
return the other operand when one operand is the literal identity. The
dropped operand is a literal, so evaluation of the kept operand is
unchanged.

Example (rescript):
    let addZero = (x: int) => x + 0
    let subZero = (x: int) => x - 0
    let mulOne = (x: int) => x * 1
    let divOne = (x: int) => x / 1

Before (js):
    function addZero(x) {
      return x + 0 | 0;
    }

    function subZero(x) {
      return x - 0 | 0;
    }

    function mulOne(x) {
      return (x << 0);
    }

    function divOne(x) {
      return x / 1 | 0;
    }

After (js):
    function addZero(x) {
      return x;
    }

    function subZero(x) {
      return x;
    }

    function mulOne(x) {
      return x;
    }

    function divOne(x) {
      return x;
    }

Source: `bsc Example.res` (CommonJS, to stdout) with bsc built from
aa2078e (Before) and from this commit (After); function bodies shown, the
module header and exports are identical.

Changelog entry: - Compile integer `x + 0`, `0 + x`, `x - 0`, `x * 1`,
    `1 * x` and `x / 1` to `x`. #8862

Review: Can an `int` operand hold a value outside the 32-bit range? Every
    int the compiler computes is already truncated: compiling `-x`,
    `Int.fromFloat(f)` and `Int.shiftRightUnsigned(x, y)` with this commit
    gives `-x | 0`, `f | 0` and `(x >>> y) | 0`. A value outside the range
    reaches an `int` only through an external or `%raw` typed `int`; for
    those the result changes, for example `Number.MAX_SAFE_INTEGER + 0` gave
    -1 and gives 9007199254740991. The compiler already relies on the same
    assumption: `Plsrint` by the literal 0 compiles to the operand
    (compiler/core/lam_compile_primitive.ml, `[e1; 0] -> e1`), dropping
    `(x >>> 0) | 0`.

Review: Does `x / 1` still check for division by zero? The divisor is the
    literal 1, so no check applies; the existing rule for a nonzero literal
    divisor already skipped the runtime `div` call.

Area: codegen tests
Changelog: polish
Output: changed integer `+ 0`, `0 +`, `- 0`, `* 1`, `1 *`, `/ 1` on a
 non-literal operand, now the bare operand; in tests/tests/src one existing
 snapshot changes (gpr_3609_test.mjs, `0 + state._0 | 0` -> `state._0`),
 and the stdlib build (make lib) emits no changed JS
Formats: none
Tests: +tests/tests/src/int_identity_ops_test.res
 +tests/tests/src/int_identity_ops_test.mjs
 ~tests/tests/src/gpr_3609_test.mjs
Ran: make (dune exit 0); make lib (exit 0); make test (exit 0: 269 ounit
 tests OK, tests/tests mocha 606 passing including Int_identity_ops_test,
 stdlib mocha 744 passing); make checkformat (exit 0); yarn changelog:check
 (passes)
Depends: #8861 (stacked)
PR: #8862
Signed-off-by: Cristiano Calcagno <ccrisccris@gmail.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cristianoc cristianoc mentioned this pull request Oct 14, 2026
Closed
@cristianoc
cristianoc marked this pull request as ready for review October 14, 2026 07:14
@cristianoc @claude
Render review pages from structured commit messages
1d51b6e 
A commit record is a commit message whose prose sections and git trailers
answer the questions reviewers of this repository ask on pull requests:
which layer changed, the changelog category and entry, the generated
JavaScript before and after, formatter, marshaled-format, PPX and editor
effects, error-catalog rows, tests and the commands that ran.
.claude/skills/records/SKILL.md defines the format and links each field to
the pull requests whose review threads raised it.
scripts/commitRecords.js renders one card per record, checks each record
against its own diff (changelog fragment, modified generated JavaScript,
magic-number bumps, v0 PPX bridge fixtures, ERROR_VARIANTS.md), and for a
range adds a summary, a changelog draft and the files that more than one
commit changes. The page is one HTML file with no script.

Example (sh):
    node scripts/commitRecords.js --out review.html master..my-branch

Changelog entry: - Add commit records: a commit message format that answers
    recurring review questions, and `scripts/commitRecords.js`, which renders
    review pages from them. #8861

Review: Does the renderer need the compiler? No: it reads git (messages,
    notes on refs/notes/records, and diffs) and has no dependencies.

Area: scripts tests docs
Changelog: internal
Output: unchanged
Tests: +scripts/commitRecords.test.js
Ran: node --test scripts/commitRecords.test.js (2 passing); npx biome check
 on the two scripts (no findings); npx tsc (exit 0)
PR: #8861
Signed-off-by: Cristiano Calcagno <cristianoc@users.noreply.github.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cristianoc added a commit that referenced this pull request Oct 14, 2026
@cristianoc @claude
Compile integer operations with an identity operand to the operand
92a9528 
Integer `x + 0`, `0 + x`, `x - 0`, `x * 1`, `1 * x` and `x / 1` compile
to `x`. A ReScript `int` value is a 32-bit integer in JavaScript, so the
`| 0` truncation (or `<< 0` for `* 1`) that these operations emitted
leaves the value unchanged. The JavaScript expression builders in
`Js_exp_make` (`int32_add`, `int32_minus`, `int32_mul`, `int32_div`)
return the other operand when one operand is the literal identity. The
dropped operand is a literal, so evaluation of the kept operand is
unchanged.

Example (rescript):
    let addZero = (x: int) => x + 0
    let subZero = (x: int) => x - 0
    let mulOne = (x: int) => x * 1
    let divOne = (x: int) => x / 1

Before (js):
    function addZero(x) {
      return x + 0 | 0;
    }

    function subZero(x) {
      return x - 0 | 0;
    }

    function mulOne(x) {
      return (x << 0);
    }

    function divOne(x) {
      return x / 1 | 0;
    }

After (js):
    function addZero(x) {
      return x;
    }

    function subZero(x) {
      return x;
    }

    function mulOne(x) {
      return x;
    }

    function divOne(x) {
      return x;
    }

Source: `bsc Example.res` (CommonJS, to stdout) with bsc built from
aa2078e (Before) and from this commit (After); function bodies shown, the
module header and exports are identical.

Changelog entry: - Compile integer `x + 0`, `0 + x`, `x - 0`, `x * 1`,
    `1 * x` and `x / 1` to `x`. #8862

Review: Can an `int` operand hold a value outside the 32-bit range? Every
    int the compiler computes is already truncated: compiling `-x`,
    `Int.fromFloat(f)` and `Int.shiftRightUnsigned(x, y)` with this commit
    gives `-x | 0`, `f | 0` and `(x >>> y) | 0`. A value outside the range
    reaches an `int` only through an external or `%raw` typed `int`; for
    those the result changes, for example `Number.MAX_SAFE_INTEGER + 0` gave
    -1 and gives 9007199254740991. The compiler already relies on the same
    assumption: `Plsrint` by the literal 0 compiles to the operand
    (compiler/core/lam_compile_primitive.ml, `[e1; 0] -> e1`), dropping
    `(x >>> 0) | 0`.

Review: Does `x / 1` still check for division by zero? The divisor is the
    literal 1, so no check applies; the existing rule for a nonzero literal
    divisor already skipped the runtime `div` call.

Area: codegen tests
Changelog: polish
Output: changed integer `+ 0`, `0 +`, `- 0`, `* 1`, `1 *`, `/ 1` on a
 non-literal operand, now the bare operand; in tests/tests/src one existing
 snapshot changes (gpr_3609_test.mjs, `0 + state._0 | 0` -> `state._0`),
 and the stdlib build (make lib) emits no changed JS
Formats: none
Tests: +tests/tests/src/int_identity_ops_test.res
 +tests/tests/src/int_identity_ops_test.mjs
 ~tests/tests/src/gpr_3609_test.mjs
Ran: make (dune exit 0); make lib (exit 0); make test (exit 0: 269 ounit
 tests OK, tests/tests mocha 606 passing including Int_identity_ops_test,
 stdlib mocha 744 passing); make checkformat (exit 0); yarn changelog:check
 (passes)
Depends: #8861 (stacked)
PR: #8862
Signed-off-by: Cristiano Calcagno <cristianoc@users.noreply.github.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@cristianoc
cristianoc force-pushed the cristianoc/commit-records branch from 9e5f07c to 1d51b6e Compare October 14, 2026 07:14
@pkg-pr-new

pkg-pr-new Bot commented Oct 14, 2026

Copy link
Copy Markdown

Open in StackBlitz

rescript

npm i https://pkg.pr.new/rescript@8861

@rescript/belt

npm i https://pkg.pr.new/@rescript/belt@8861

@rescript/darwin-arm64

npm i https://pkg.pr.new/@rescript/darwin-arm64@8861

@rescript/darwin-x64

npm i https://pkg.pr.new/@rescript/darwin-x64@8861

@rescript/linux-arm64

npm i https://pkg.pr.new/@rescript/linux-arm64@8861

@rescript/linux-x64

npm i https://pkg.pr.new/@rescript/linux-x64@8861

@rescript/runtime

npm i https://pkg.pr.new/@rescript/runtime@8861

@rescript/win32-x64

npm i https://pkg.pr.new/@rescript/win32-x64@8861

commit: 1d51b6e

@github-actions

github-actions Bot commented Oct 14, 2026

Copy link
Copy Markdown

Developer playground preview: https://rescript-lang.github.io/rescript/dev-playground/?version=pr-8861

@cristianoc

cristianoc commented Oct 16, 2026

Copy link
Copy Markdown
Collaborator Author

Superseded by #8870, which also links a semantic diff page on each PR.

@cristianoc cristianoc closed this Oct 16, 2026
@cristianoc
cristianoc deleted the cristianoc/commit-records branch October 16, 2026 01:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@cristianoc