Web playground (WASM) for driving the complement engine from a phone - #65
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughAdds a ChangesWeb WASM Playground with gp Feature Gate
Sequence Diagram(s)sequenceDiagram
participant Browser as Browser (index.html)
participant AppJS as app.js
participant WASMModule as griff_web.wasm
participant CoreLogic as arrange_complement_varied
Browser->>AppJS: user changes controls / clicks Generate
AppJS->>WASMModule: arrange(mode, seed, offset, variation)
WASMModule->>CoreLogic: sample_part_a + ComplementSpec + VariationControl
CoreLogic-->>WASMModule: VariedComplement or VariationError
WASMModule-->>AppJS: *const u8 (JSON in linear memory)
AppJS->>WASMModule: arrange_len()
WASMModule-->>AppJS: byte length
AppJS->>AppJS: decode JSON, draw piano-roll canvas
Browser->>AppJS: click Play
AppJS->>AppJS: schedule Web Audio oscillators, animate playhead
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/Cargo.toml (1)
29-32:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-apply
unsafe_code = "forbid"in this isolated workspace.This crate is intentionally excluded from the root workspace, so it no longer inherits the root lint policy that forbids unsafe code. Add an explicit lint rule here to keep the workspace-wide guarantee intact.
Suggested manifest patch
[workspace] + +[lints.rust] +unsafe_code = "forbid"As per coding guidelines, “Forbid unsafe code workspace-wide; no exceptions without an ADR.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/Cargo.toml` around lines 29 - 32, The isolated workspace section in web/Cargo.toml no longer inherits the root workspace's lint policies since it's excluded from the root workspace. Add an explicit lint configuration to the workspace section to re-apply the unsafe_code = "forbid" policy, ensuring the workspace-wide guarantee that unsafe code is forbidden remains in effect for this isolated workspace.Source: Coding guidelines
🧹 Nitpick comments (2)
core/tests/complement.rs (2)
275-290: ⚡ Quick winStrengthen the identity-window test to validate full behavioral identity, not just pitch values.
This test currently checks only
note_pitches, but the stated contract is byte-identical behavior at full spread. Add onset/duration (or full B-track event-group) equality assertions so regressions in non-pitch fields are caught.Proposed test tightening
assert_eq!( note_pitches(&varied.complement.score, varied.complement.part_b_index), note_pitches(&plain.score, plain.part_b_index), "VariationControl::FULL must be the identity window", ); + assert_eq!( + note_onsets(&varied.complement.score, varied.complement.part_b_index), + note_onsets(&plain.score, plain.part_b_index), + "FULL must preserve onset placement", + ); + assert_eq!( + note_durations(&varied.complement.score, varied.complement.part_b_index), + note_durations(&plain.score, plain.part_b_index), + "FULL must preserve note durations", + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/tests/complement.rs` around lines 275 - 290, The test variation_full_spread_equals_plain_arrange currently only validates pitch equality via note_pitches when comparing the output of arrange_complement_varied with VariationControl::FULL against the plain arrange_complement result. To properly validate the identity-window contract stating that FULL spread should produce byte-identical behavior, add additional assertions beyond note_pitches to verify onset and duration values (or the complete B-track event-group structure) also match between varied.complement and plain. This ensures regressions in non-pitch fields are caught by the test.
260-389: ⚡ Quick winVariationControl coverage is limited to
RhythmLock; add coverage for the other ladder-threaded modes.Implementation threads control through
RegisterContrastandCallResponsetoo, but the new variation tests only exercise one mode. Add at least one LOCKED/FULL assertion per mode to protect the wiring contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/tests/complement.rs` around lines 260 - 389, The VariationControl tests currently only exercise the RhythmLock mode via the RHYTHM_LOCK constant, but the implementation threads control through RegisterContrast and CallResponse modes as well. Add ComplementSpec constants for RegisterContrast and CallResponse modes (similar to the existing RHYTHM_LOCK constant), then create or extend test functions to exercise arrange_complement_varied with at least one VariationControl::LOCKED assertion and one VariationControl::FULL assertion for each of these additional modes to ensure the wiring contract is protected across all ladder-threaded modes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/web.yml:
- Around line 12-15: The workflow-level permissions block currently includes
`pages: write` and `id-token: write` permissions that are only required by the
deploy job, not the build job. Remove `pages: write` and `id-token: write` from
the top-level permissions section, keeping only `contents: read` at the workflow
level. Then add a dedicated `permissions` section to the deploy job in the jobs
section with the full set of permissions it needs: `contents: read`, `pages:
write`, and `id-token: write`. This ensures the build job operates with minimal
required permissions while the deploy job has access to the elevated permissions
it requires.
- Around line 26-51: Replace all GitHub Actions version references with
full-length commit SHAs to prevent supply-chain attacks from mutable tag
references. Update actions/checkout@v4 on line 26 to use its commit SHA,
actions/cache@v4 on line 30 to use its commit SHA,
actions/upload-pages-artifact@v3 on line 39 to use its commit SHA, and
actions/deploy-pages@v4 on line 51 to use its commit SHA. Additionally, add
persist-credentials: false as an option to the checkout action on line 26 to
prevent the GITHUB_TOKEN from being stored in the git config, reducing the risk
of token exfiltration.
In `@core/src/complement.rs`:
- Around line 90-104: The current commit combines both implementation and tests
together, which violates Rust coding guidelines. Split this into two separate
commits: first, create a tests-only commit containing all test coverage for the
new public API (VariationControl struct with its FULL and LOCKED constants,
VariedComplement, VariationError, arrange_complement_varied, and any related
test functions), written as failing tests. Then, in a second commit, add the
actual implementation of the VariationControl struct, its constants
pitch_spread, FULL, and LOCKED, along with all the supporting implementation
code. This ensures tests are defined before the implementation they verify.
In `@docs/adr/0024-web-wasm-frontend-for-mobile.md`:
- Around line 38-41: Update the wording in the M1 MVP description to correctly
reflect the number of exported C-ABI functions. Change "exports three C-ABI
functions" to "exports two C-ABI functions" and list the actual exported
contract: the two C-ABI functions arrange and arrange_len, plus the linear
memory export. This ensures the ADR text accurately matches the implemented
contract.
In `@web/src/lib.rs`:
- Around line 185-191: The error text is being directly interpolated into a JSON
string using `{:?}` format without escaping special characters. This can produce
invalid JSON if the error message contains quotes, backslashes, or control
characters, causing browser parsing to fail. Before embedding the error into the
JSON output in the error branch of the write! macro, properly escape the error
string to ensure it produces valid JSON. Consider using a JSON escaping function
or library that converts the error debug representation to a properly escaped
JSON string value.
- Around line 209-264: The public C-ABI functions arrange and arrange_len were
introduced in the same commit as their test cases, violating the commit split
requirement that tests must be committed separately from their implementation.
To fix this, reset your current commit and split it into two: first commit the
test module (the tests block containing
every_mode_emits_part_a_and_well_formed_header,
counter_melody_succeeds_on_the_uniform_sample,
pitch_spread_changes_rhythm_lock_output, and deterministic_for_identical_args),
then in a separate commit add the arrange and arrange_len public extern
functions along with the supporting code they depend on.
---
Outside diff comments:
In `@web/Cargo.toml`:
- Around line 29-32: The isolated workspace section in web/Cargo.toml no longer
inherits the root workspace's lint policies since it's excluded from the root
workspace. Add an explicit lint configuration to the workspace section to
re-apply the unsafe_code = "forbid" policy, ensuring the workspace-wide
guarantee that unsafe code is forbidden remains in effect for this isolated
workspace.
---
Nitpick comments:
In `@core/tests/complement.rs`:
- Around line 275-290: The test variation_full_spread_equals_plain_arrange
currently only validates pitch equality via note_pitches when comparing the
output of arrange_complement_varied with VariationControl::FULL against the
plain arrange_complement result. To properly validate the identity-window
contract stating that FULL spread should produce byte-identical behavior, add
additional assertions beyond note_pitches to verify onset and duration values
(or the complete B-track event-group structure) also match between
varied.complement and plain. This ensures regressions in non-pitch fields are
caught by the test.
- Around line 260-389: The VariationControl tests currently only exercise the
RhythmLock mode via the RHYTHM_LOCK constant, but the implementation threads
control through RegisterContrast and CallResponse modes as well. Add
ComplementSpec constants for RegisterContrast and CallResponse modes (similar to
the existing RHYTHM_LOCK constant), then create or extend test functions to
exercise arrange_complement_varied with at least one VariationControl::LOCKED
assertion and one VariationControl::FULL assertion for each of these additional
modes to ensure the wiring contract is protected across all ladder-threaded
modes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 72d2cfc0-84b8-4d62-b6ef-970720c3ca03
📒 Files selected for processing (18)
.github/workflows/web.ymlCargo.tomlcore/Cargo.tomlcore/src/complement.rscore/src/import.rscore/src/lib.rscore/tests/complement.rsdocs/adr/0023-variation-control-for-complement.mddocs/adr/0024-web-wasm-frontend-for-mobile.mddocs/adr/README.mdweb/.gitignoreweb/Cargo.tomlweb/README.mdweb/build.shweb/src/lib.rsweb/static/app.jsweb/static/index.htmlweb/static/style.css
| permissions: | ||
| contents: read | ||
| pages: write | ||
| id-token: write |
There was a problem hiding this comment.
Scope elevated permissions to the deploy job only.
Line 14 (pages: write) and Line 15 (id-token: write) are set workflow-wide; the build job does not require them. Move these permissions to jobs.deploy.permissions and keep top-level/job-build permissions read-only.
Suggested permission scoping
-permissions:
- contents: read
- pages: write
- id-token: write
+permissions:
+ contents: read
@@
build:
runs-on: ubuntu-latest
+ permissions:
+ contents: read
@@
deploy:
needs: build
runs-on: ubuntu-latest
+ permissions:
+ pages: write
+ id-token: write🧰 Tools
🪛 zizmor (1.25.2)
[error] 14-14: overly broad permissions (excessive-permissions): pages: write is overly broad at the workflow level
(excessive-permissions)
[error] 15-15: overly broad permissions (excessive-permissions): id-token: write is overly broad at the workflow level
(excessive-permissions)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/web.yml around lines 12 - 15, The workflow-level
permissions block currently includes `pages: write` and `id-token: write`
permissions that are only required by the deploy job, not the build job. Remove
`pages: write` and `id-token: write` from the top-level permissions section,
keeping only `contents: read` at the workflow level. Then add a dedicated
`permissions` section to the deploy job in the jobs section with the full set of
permissions it needs: `contents: read`, `pages: write`, and `id-token: write`.
This ensures the build job operates with minimal required permissions while the
deploy job has access to the elevated permissions it requires.
Source: Linters/SAST tools
| - uses: actions/checkout@v4 | ||
| - name: Install wasm target | ||
| run: rustup target add wasm32-unknown-unknown | ||
| - name: Cache cargo | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: | | ||
| ~/.cargo/registry | ||
| ~/.cargo/git | ||
| web/target | ||
| key: web-wasm-${{ hashFiles('web/Cargo.toml', 'core/Cargo.toml') }} | ||
| - name: Build playground | ||
| run: ./web/build.sh | ||
| - uses: actions/upload-pages-artifact@v3 | ||
| with: | ||
| path: web/dist | ||
|
|
||
| deploy: | ||
| needs: build | ||
| runs-on: ubuntu-latest | ||
| environment: | ||
| name: github-pages | ||
| url: ${{ steps.deploy.outputs.page_url }} | ||
| steps: | ||
| - id: deploy | ||
| uses: actions/deploy-pages@v4 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/web.yml | head -60Repository: PhysShell/griff
Length of output: 1648
🌐 Web query:
GitHub Actions security best practices pin commit SHA vs tag references official documentation
💡 Result:
GitHub’s official security guidance strongly recommends pinning GitHub Actions to a full-length commit SHA as the only way to ensure an action is used as an immutable release [1][2]. Key security considerations include: 1. Immutability: A commit SHA uniquely and immutably identifies a specific version of an action’s code [3][4]. In contrast, tags (e.g., @v1) and branch references (e.g., @main) are mutable; they can be moved or deleted by an action's maintainers at any time, potentially introducing malicious or breaking code into your workflows without your knowledge [1][3][5]. 2. Risk Mitigation: Pinning to a SHA mitigates the risk of supply chain attacks where a compromised account or repository adds backdoors or malicious payloads to an existing tag [1][6]. While tags rely on trust in the creator, SHAs provide cryptographic assurance that you are running the exact code you reviewed [1][7]. 3. Organizational Enforcement: GitHub supports organizational and repository-level policies that mandate the use of full-length commit SHAs [1][8]. When enforced, any workflow attempting to use an action pinned to a tag or branch will fail [8][4]. 4. Best Practices for Maintenance: Because commit SHAs are opaque and do not automatically receive updates, it is recommended to include the human-readable tag as a comment in your YAML file to maintain clarity [6][7]. Many organizations use automation tools like Renovate or similar dependency managers to automatically create pull requests that update SHA pins when the referenced tags move, balancing security with ease of maintenance [8][6][9]. Recommendation: Always pin third-party actions to a full-length commit SHA [1][6]. While you may continue to use tags if you explicitly trust the creator, understand that this exposes your pipeline to potential upstream compromises [1][5]. If you maintain your own actions, ensure that any internal references to sub-actions are also pinned to SHAs to prevent breaking workflows for users who have enforced SHA-pinning policies [4].
Citations:
- 1: https://docs.github.com/en/actions/reference/security/secure-use
- 2: https://codeql.github.com/codeql-query-help/actions/actions-unpinned-tag/
- 3: https://www.stepsecurity.io/blog/pinning-github-actions-for-enhanced-security-a-complete-guide
- 4: https://www.romainlespinasse.dev/posts/github-actions-commit-sha-pinning/
- 5: https://safeguard.sh/resources/blog/github-actions-pinning-sha-vs-tag
- 6: https://architecture.lullabot.com/adr/20260505-pin-github-actions-to-hash/
- 7: https://nefariousplan.com/posts/tj-actions-mutable-tags-were-always-a-lie
- 8: https://github.blog/changelog/2025-08-15-github-actions-policy-now-supports-blocking-and-sha-pinning-actions/
- 9: https://mikael.barbero.tech/blog/post/2026-03-24-stop-trusting-mutable-references/
🌐 Web query:
GitHub Actions checkout persist-credentials false security best practices token exposure
💡 Result:
Setting persist-credentials: false in the actions/checkout GitHub Action is a security best practice recommended to prevent unintended token exposure [1][2][3]. By default, actions/checkout sets persist-credentials to true, which stores the GITHUB_TOKEN (or a provided personal access token) in the local.git/config file on the runner [4][5][3]. This enables subsequent steps in the same job to execute authenticated Git commands (such as git fetch or push) [4][6][7]. However, this also makes the token accessible to any process, script, or third-party action running in that job [8][1][9]. The primary security risks associated with keeping the default (persist-credentials: true) include: 1. Artifact Exposure (ArtiPACKED): If a workflow uploads the repository workspace as an artifact (e.g., using actions/upload-artifact), the.git directory—containing the persisted credentials—may be included [10][11][12]. If this artifact is public or accessible to unauthorized users, the token can be exfiltrated and misused [2][10][12]. 2. Increased Attack Surface: Any compromised or malicious third-party action within the workflow can read the.git/config file to obtain the token, potentially escalating privileges beyond what was intended for that specific action [1][9]. Recommendation: - Set persist-credentials: false for any checkout step that does not explicitly require authenticated Git operations (e.g., jobs focused solely on testing, building, or scanning) [1][9]. - If your workflow must perform Git operations (like pushing commits), consider using persist-credentials: false and managing authentication through other methods, or ensure that artifacts are configured to strictly exclude the.git directory (e.g., by using paths to target only specific build output folders) [2][10][12]. Using security auditing tools like zizmor or custom security policies can help detect and enforce the use of persist-credentials: false across your workflows [12][13].
Citations:
- 1: https://actsense.dev/vulnerabilities/unsafe_checkout/
- 2: https://sisaku-security.github.io/lint/docs/advisory/ghsa-x6gv-2rvh-qmp6/
- 3: https://yossarian.net/til/post/actions-checkout-can-leak-github-credentials/
- 4: https://github.com/actions/checkout/?tab=readme-ov-file
- 5: https://github.com/actions/checkout/tree/refs/heads/main
- 6: https://github.com/actions/checkout/blob/master/adrs/0153-checkout-v2.md
- 7: https://github.com/actions/checkout
- 8: Remove
persist-credentialsor change the default tofalseactions/checkout#485 - 9: Add persist-credentials: false to GitHub Actions checkout steps aquasecurity/trivy#10305
- 10: https://sisaku-security.github.io/lint/docs/rules/artipacked/
- 11: https://actsense.dev/vulnerabilities/artifact_exposure_risk/
- 12: [plan] Fix artipacked credential persistence vulnerability in daily-copilot-token-report github/gh-aw#18285
- 13: https://earthly.dev/lunar/guardrails/github-actions/checkout-no-persist-credentials/
Pin all GitHub Actions by commit SHA and disable checkout credential persistence.
Lines 26, 30, 39, and 51 currently use tag refs (@v*) instead of full-length commit SHAs. Tag references are mutable—they can be moved, deleted, or compromised by a malicious maintainer—exposing your workflow to supply-chain attacks. Additionally, set persist-credentials: false on the checkout step to prevent the GITHUB_TOKEN from being stored in .git/config, which reduces the risk of token exfiltration if a third-party action is compromised or if artifacts leak.
Suggested hardening patch
- - uses: actions/checkout@v4
+ - uses: actions/checkout@<full_commit_sha>
+ with:
+ persist-credentials: false
@@
- uses: actions/cache@v4
+ uses: actions/cache@<full_commit_sha>
@@
- - uses: actions/upload-pages-artifact@v3
+ - uses: actions/upload-pages-artifact@<full_commit_sha>
@@
- uses: actions/deploy-pages@v4
+ uses: actions/deploy-pages@<full_commit_sha>🧰 Tools
🪛 zizmor (1.25.2)
[warning] 26-26: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 26-26: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 30-30: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 39-39: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 51-51: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/web.yml around lines 26 - 51, Replace all GitHub Actions
version references with full-length commit SHAs to prevent supply-chain attacks
from mutable tag references. Update actions/checkout@v4 on line 26 to use its
commit SHA, actions/cache@v4 on line 30 to use its commit SHA,
actions/upload-pages-artifact@v3 on line 39 to use its commit SHA, and
actions/deploy-pages@v4 on line 51 to use its commit SHA. Additionally, add
persist-credentials: false as an option to the checkout action on line 26 to
prevent the GITHUB_TOKEN from being stored in the git config, reducing the risk
of token exfiltration.
Source: Linters/SAST tools
| pub struct VariationControl { | ||
| /// Fraction of the band's scale ladder B may use, `0.0..=1.0`. `0.0` pins | ||
| /// every note to the band's anchor degree (a static line, still locked to | ||
| /// A's grid); `1.0` uses the whole band — the unconstrained default that | ||
| /// matches [`arrange_complement`]. | ||
| pub pitch_spread: f64, | ||
| } | ||
|
|
||
| impl VariationControl { | ||
| /// The identity control: the whole band, i.e. [`arrange_complement`]'s | ||
| /// behaviour. | ||
| pub const FULL: Self = Self { pitch_spread: 1.0 }; | ||
| /// A static line: B collapses onto the band's anchor degree. | ||
| pub const LOCKED: Self = Self { pitch_spread: 0.0 }; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Chronological commits touching implementation or tests =="
git log --reverse --date=short \
--pretty=format:'COMMIT %H %ad %s' \
--name-only -- core/src/complement.rs core/tests/complement.rs
echo
echo "== Commits that touched BOTH implementation and tests in the same commit =="
git log --date=short --pretty=format:'%H|%ad|%s' \
--name-only -- core/src/complement.rs core/tests/complement.rs \
| awk '
BEGIN { RS=""; FS="\n" }
{
header=$1; impl=0; test=0;
for (i=2; i<=NF; i++) {
if ($i=="core/src/complement.rs") impl=1;
if ($i=="core/tests/complement.rs") test=1;
}
if (impl && test) print header;
}'Repository: PhysShell/griff
Length of output: 454
Revert to separate commits for tests and implementation.
Your commit 72ae97e7dad3605fbe4ad4cc535aeaf9f71995e3 (2026-06-16) modified both core/src/complement.rs and core/tests/complement.rs together. The Rust coding guidelines require that new pub fn or pub struct implementations must be committed separately from their test coverage.
To comply:
- Create a tests-only commit first with failing tests for the new public API (VariationControl, VariedComplement, VariationError, arrange_complement_varied, and other affected functions)
- Then create a separate commit with the implementation
Affected lines: 90-104, 116-134, 698-716
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@core/src/complement.rs` around lines 90 - 104, The current commit combines
both implementation and tests together, which violates Rust coding guidelines.
Split this into two separate commits: first, create a tests-only commit
containing all test coverage for the new public API (VariationControl struct
with its FULL and LOCKED constants, VariedComplement, VariationError,
arrange_complement_varied, and any related test functions), written as failing
tests. Then, in a second commit, add the actual implementation of the
VariationControl struct, its constants pitch_spread, FULL, and LOCKED, along
with all the supporting implementation code. This ensures tests are defined
before the implementation they verify.
Source: Coding guidelines
| 2. **The M1 MVP is a thin, throwaway front, not egui** — an *import-free* | ||
| `cdylib` (`web/`, no `wasm-bindgen`, no framework) that exports three C-ABI | ||
| functions, plus a static `index.html` + `app.js` that loads the `.wasm` with | ||
| `WebAssembly.instantiate(bytes, {})` and marshals a small JSON result through |
There was a problem hiding this comment.
ABI wording mismatch: this MVP exports two C-ABI functions, not three.
Please update the ADR text to match the implemented/exported contract (arrange, arrange_len, plus linear memory export).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/adr/0024-web-wasm-frontend-for-mobile.md` around lines 38 - 41, Update
the wording in the M1 MVP description to correctly reflect the number of
exported C-ABI functions. Change "exports three C-ABI functions" to "exports two
C-ABI functions" and list the actual exported contract: the two C-ABI functions
arrange and arrange_len, plus the linear memory export. This ensures the ADR
text accurately matches the implemented contract.
| Err(e) => { | ||
| // Surface the typed error; still return A so the page can draw it. | ||
| let _ = write!( | ||
| json, | ||
| "\"realized_spread\":0,\"error\":\"{:?}\",\"tracks\":[", | ||
| e | ||
| ); |
There was a problem hiding this comment.
Escape error text before embedding it in JSON.
The error branch interpolates {:?} directly into a quoted JSON string. If the debug text contains quotes/backslashes/control chars, the payload becomes invalid JSON and the browser parse path fails.
Suggested fix
Err(e) => {
// Surface the typed error; still return A so the page can draw it.
+ let err = json_escape(&format!("{e:?}"));
let _ = write!(
json,
- "\"realized_spread\":0,\"error\":\"{:?}\",\"tracks\":[",
- e
+ "\"realized_spread\":0,\"error\":\"{}\",\"tracks\":[",
+ err
);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/lib.rs` around lines 185 - 191, The error text is being directly
interpolated into a JSON string using `{:?}` format without escaping special
characters. This can produce invalid JSON if the error message contains quotes,
backslashes, or control characters, causing browser parsing to fail. Before
embedding the error into the JSON output in the error branch of the write!
macro, properly escape the error string to ensure it produces valid JSON.
Consider using a JSON escaping function or library that converts the error debug
representation to a properly escaped JSON string value.
| pub extern "C" fn arrange(mode: u32, seed: u32, offset: i32, variation: f32) -> *const u8 { | ||
| let json = build_json(mode, u64::from(seed), offset, variation); | ||
| OUT.with(|o| { | ||
| *o.borrow_mut() = json.into_bytes(); | ||
| o.borrow().as_ptr() | ||
| }) | ||
| } | ||
|
|
||
| /// Length in bytes of the JSON stored by the last [`arrange`] call. | ||
| #[no_mangle] | ||
| pub extern "C" fn arrange_len() -> usize { | ||
| OUT.with(|o| o.borrow().len()) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::build_json; | ||
|
|
||
| #[test] | ||
| fn every_mode_emits_part_a_and_well_formed_header() { | ||
| for mode in 0..6 { | ||
| let j = build_json(mode, 5, 0, 1.0); | ||
| assert!( | ||
| j.starts_with("{\"ppqn\":480,\"tempo\":120"), | ||
| "mode {mode}: {j:.60}" | ||
| ); | ||
| assert!(j.contains("\"tracks\":["), "mode {mode}: has tracks"); | ||
| assert!(j.contains("\"role\":\"a\""), "mode {mode}: has part A"); | ||
| assert!(j.ends_with('}'), "mode {mode}: closed object"); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn counter_melody_succeeds_on_the_uniform_sample() { | ||
| // mode 5 = counter_melody; the sample is uniform 4/4, so no NonUniformTimeline. | ||
| let j = build_json(5, 0, 0, 1.0); | ||
| assert!(j.contains("\"error\":null"), "expected success: {j:.120}"); | ||
| assert!(j.contains("\"role\":\"b\""), "counter_melody emits part B"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn pitch_spread_changes_rhythm_lock_output() { | ||
| // mode 0 = rhythm_lock: the knob must move B's pitches. | ||
| let locked = build_json(0, 5, 0, 0.0); | ||
| let full = build_json(0, 5, 0, 1.0); | ||
| assert_ne!( | ||
| locked, full, | ||
| "pitch_spread must change a grid-locked complement" | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn deterministic_for_identical_args() { | ||
| assert_eq!(build_json(5, 7, -12, 0.5), build_json(5, 7, -12, 0.5)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Public C-ABI function line commits:"
git blame -L 208,220 -- web/src/lib.rs | awk '{print $1}' | sort -u
echo
echo "Test block line commits:"
git blame -L 223,264 -- web/src/lib.rs | awk '{print $1}' | sort -u
echo
echo "Overlapping commits between API and tests (non-empty needs review):"
comm -12 \
<(git blame -L 208,220 -- web/src/lib.rs | awk '{print $1}' | sort -u) \
<(git blame -L 223,264 -- web/src/lib.rs | awk '{print $1}' | sort -u)
echo
echo "Chronological file history for manual confirmation:"
git log --reverse --oneline -- web/src/lib.rsRepository: PhysShell/griff
Length of output: 270
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Commit blame for public C-ABI functions (lines 208-220) ==="
git blame -L 208,220 -- web/src/lib.rs | head -5
echo
echo "=== Commit blame for test block (lines 223-264) ==="
git blame -L 223,264 -- web/src/lib.rs | head -5
echo
echo "=== Get unique commits for each section ==="
PUBLIC_COMMITS=$(git blame -L 208,220 -- web/src/lib.rs | awk '{print $1}' | sort -u)
TEST_COMMITS=$(git blame -L 223,264 -- web/src/lib.rs | awk '{print $1}' | sort -u)
echo "Public function commits: $PUBLIC_COMMITS"
echo "Test block commits: $TEST_COMMITS"
if [ "$PUBLIC_COMMITS" = "$TEST_COMMITS" ]; then
echo "SAME COMMITS: Both sections were introduced in the same commit(s)"
else
echo "DIFFERENT COMMITS: Sections were introduced separately"
fi
echo
echo "=== Full commit log for web/src/lib.rs ==="
git log --oneline -- web/src/lib.rs | head -20Repository: PhysShell/griff
Length of output: 1234
Commit split violation confirmed: public API and tests introduced together.
The public C-ABI functions arrange and arrange_len (lines 209–217) were introduced in the same commit as the tests covering them (lines 223–264). Both sections carry commit hash 72ae97e ("Add web playground (WASM) MVP for mobile testing"), violating the requirement that "new pub fn or pub struct implementation" not be committed alongside their covering tests.
The commits must be split: tests should be committed first (in a failing state if possible per TDD), followed by a separate commit introducing the public functions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/lib.rs` around lines 209 - 264, The public C-ABI functions arrange
and arrange_len were introduced in the same commit as their test cases,
violating the commit split requirement that tests must be committed separately
from their implementation. To fix this, reset your current commit and split it
into two: first commit the test module (the tests block containing
every_mode_emits_part_a_and_well_formed_header,
counter_melody_succeeds_on_the_uniform_sample,
pitch_spread_changes_rhythm_lock_output, and deterministic_for_identical_args),
then in a separate commit add the arrange and arrange_len public extern
functions along with the supporting code they depend on.
Source: Coding guidelines
ADR-0024: a browser front so the complement engine can be driven — and heard —
from a phone, no install. The MVP is a deliberately thin, throwaway front
(ADR-0024 §2): an import-free `cdylib` (`web/`, no wasm-bindgen, no Trunk)
exporting three C-ABI functions, plus a static HTML/canvas/WebAudio page that
loads the .wasm with `WebAssembly.instantiate(bytes, {})` and reads a JSON
result from linear memory. Live controls for mode, seed, register offset, and
the ADR-0023 pitch_spread knob; deterministic.
To make the module import-free, gate griff-core's Guitar Pro importer behind a
default-on `gp` feature (guitarpro/zip → time/getrandom → wasm-bindgen). `web`
builds with `default-features = false`, leaving a ~90 KiB import-free wasm; the
CLI and tests keep `gp` on and are unchanged (verified clippy/tests on and off).
Host-side unit tests cover the JSON path (every mode well-formed, counter_melody
succeeds on the uniform sample, pitch_spread moves rhythm_lock, determinism).
Includes web/build.sh, a GitHub Pages workflow, and README. Audio is a
placeholder WebAudio synth; the egui frontend (ADR-0016) and a SoundFont are M2.
Stacked on the VariationControl branch (PR #63), which it depends on.
https://claude.ai/code/session_01TTUbGjzD8ysnVnCJnZJE95
72ae97e to
79bcb1a
Compare
PhysShell
changed the title
…I count CodeRabbit on #65: - web/src/lib.rs: json_escape the error Debug repr before embedding it in the result JSON (quotes/backslashes would otherwise break the browser parse). - .github/workflows/web.yml: drop pages/id-token from the workflow-level permissions (keep contents: read), scope them to the deploy job, and set persist-credentials: false on checkout (zizmor: excessive-permissions, artipacked). - docs/adr/0024 + web/README: the crate exports two C-ABI functions (arrange, arrange_len) plus the linear memory, not three. https://claude.ai/code/session_01TTUbGjzD8ysnVnCJnZJE95
2 participants
A browser front for the complement arranger, built so the engine can be driven — and heard — from a phone, no install. ADR-0024 (added here). Rebased onto
mainafter #64, so the diff is only the web/gp-gate/ADR-0024 files.What it is (MVP, ADR-0024 §2)
A deliberately thin, throwaway front — no
wasm-bindgen, no framework.griff-webis an import-freecdylibexporting three C-ABI functions; the static page (web/static/) loads the.wasmwithWebAssembly.instantiate(bytes, {})and marshals a small JSON result through linear memory. The canonicaleguifrontend (ADR-0016) replaces it at M2.Live controls: mode (incl.
counter_melody), seed, register offset, and pitch spread (the merged ADR-0023VariationControl, audible on the grid-locked modes). A canvas piano-roll (A blue / B amber) + WebAudio playback. Deterministic: same controls → same result.How it stays import-free (~90 KiB)
Adds a default-on
gpfeature togriff-core, gating the Guitar Pro importer (guitarpro/zip→time/getrandom→wasm-bindgen/js-sys).webdepends on core withdefault-features = false, dropping that whole subtree — the module needs no JS glue and instantiates with{}. The CLI and tests keepgpon and are unchanged (host build + clippy verified both on and off).Infra
web/build.sh→web/dist(justcargo build --target wasm32+ copy static files; no Trunk/wasm-bindgen)..github/workflows/web.yml→ builds and deploys to GitHub Pages (enable Pages → "GitHub Actions" in repo settings).web/is wasm32-only and excluded from the root workspace (likefuzz/), so stable--workspacebuilds/clippy/tests never touch it.Verification
gpon); web wasm builds import-free (0 imports); 4 host-side unit tests cover the JSON path (every mode well-formed,counter_melodysucceeds,pitch_spreadmovesrhythm_lock, determinism);clippy/fmtclean.Notes / next
#[cfg(test)]unit tests (idiomatic for a small cdylib) rather than a separate test file.https://claude.ai/code/session_01TTUbGjzD8ysnVnCJnZJE95
Summary by CodeRabbit
New Features
Chores