Skip to content

ci: add pre-merge quality gates, supply-chain scanning, and deployments - #105

Merged
scotej merged 13 commits into
mainfrom
ci/premerge-quality-gates
Jul 27, 2026
Merged

ci: add pre-merge quality gates, supply-chain scanning, and deployments#105
scotej merged 13 commits into
mainfrom
ci/premerge-quality-gates

Conversation

@scotej

@scotej scotej commented Jul 27, 2026

Copy link
Copy Markdown
Owner

Closes #102.

CI proved the code compiled and the existing guards passed. It did not
check the workflows themselves, anything about the dependency graph, or
either of the two compatibility surfaces CLAUDE.md calls cross-version
contracts — and nothing produced an artifact anyone could install before a
release tag existed. This adds those, plus the deployments.

Every blocking gate below was run locally against this tree before being
made blocking
. That mattered: three of them were red on the first try.

Blocking pre-merge gates (new)

Gate Catches Verified
actionlint Broken workflow YAML, shellcheck defects in inline run: blocks exit 0 — found 1 real issue in release.yml, fixed here
zizmor Unpinned third-party actions, injection sinks, credential persistence exit 0 at high/high with .github/zizmor.yml
cargo-deny Rust advisories, licences, banned crates, non-crates.io sources advisories ok, bans ok, licenses ok, sources ok
dependency-review Vulnerabilities this PR adds scores the diff, so the existing backlog doesn't block
typos Misspellings, especially in src/strings.ts exit 0 with _typos.toml
check-migrations Edited/unregistered/misnumbered SQLite migrations exit 0; all 3 failure modes tested
check-stories A component landing with no Storybook story exit 0 (54 components, 69 stories)
version lockstep A partial version bump across the 5 tracked files exit 0
PR title Non-conventional-commit titles regex tested against real history

All pre-merge checks aggregates them, so adding a job later extends the
gate without editing the protection rule.

Three gates that were red on the first attempt

Worth calling out, because "add the gate" and "add a gate that passes" are
different jobs:

  1. zizmor flagged 12 high findings — all actions/checkout@v6-style
    major tags the repo deliberately allows. Fixed by encoding the actual
    policy (actions/* and github/* may float, everything else must be
    hash-pinned) rather than by lowering severity. It now enforces the
    paragraph at the top of ci.yml instead of contradicting it.
  2. typos flagged 24 — every one a false positive (unparseable,
    NULLable, base64 fixtures, AB-BA lock ordering). Each checked
    against the source individually, then re-verified that real typos are
    still caught.
  3. cargo-deny check advisories failed on 8 findings. Six are
    transitive abandoned crates no change here can fix (unmaintained = "workspace" scopes that to our own deps); two are real quick-xml DoS
    advisories, pinned by ID with reasons. Anything new still fails
    that's the point, and why it isn't just continue-on-error.

Advisory-only by design: npm audit (red today, ISSUES.md I19 — a
permanently red gate teaches you to merge past red), link checking (other
people's sites moving must not block a code PR), and a new wider clippy
pass that measures the backlog the current correctness-only invocation
has never shown.

Deployments

  • Storybook → GitHub Pages from main. Pages is enabled and pointed at
    build_type: workflow; the site will be https://scotej.github.io/studyvis/.
    Each PR also gets the same build as an artifact, reusing the Storybook
    build CI already does rather than building twice.
  • build-installers label on a PR builds real macOS + Windows bundles
    you can install and smoke-test. Label-gated because it's 30–60 min per
    platform. createUpdaterArtifacts is switched off, which is both what
    lets it run without signing secrets and what guarantees no PR artifact
    can ever be consumed by a client's updater. pull_request, never
    pull_request_target; the Rust cache is read-only for these jobs.

Also

CodeQL (javascript-typescript, rust, actions, all build-mode none),
a weekly maintenance workflow (OSV over both lockfiles, relay health,
Scorecard), Dependabot for npm/cargo/actions, PR + issue templates, and
SECURITY.md.

One finding worth your attention

OSV surfaced GHSA-7gmj-67g7-phm9 against the pinned tauri 2.11.0:
origin confusion allowing remote pages to invoke local-only IPC commands,
CVSS 8.8, fixed in 2.11.1. cargo-deny does not see it — the
advisory is GitHub-Advisory-Database-only and RustSec doesn't carry it,
which is precisely why both scanners are here.

Left open deliberately and ledgered as I76: a Tauri bump is a Rust
change that can't be compiled on this box, so it wants its own PR and its
own CI run rather than riding along with CI config. Dependabot will open
it (tauri* is excluded from the routine grouping so it lands reviewable),
and the weekly OSV scan keeps reporting until it does.

Manual test

  • Every blocking gate run locally against this tree — see the table.
  • npm run lint, test (77 files / 871 tests), check-tokens,
    check-strings, check-migrations, check-stories,
    tsc --noEmit, format:check — all pass.
  • check-migrations tested against all three failure modes (edited
    migration, unregistered file, sequence gap) and that --update
    refuses to launder an edit as an addition.
  • check-stories' stale-exemption detector tested — it caught a wrong
    entry in my own first draft.
  • Desktop app not walked — n/a, no app code changes.

Rust changes are limited to Cargo.toml gaining publish = false; this
box can't compile the Tauri crate, so CI is the first compiler.
Cargo.lock and package-lock.json are untouched.

Compatibility surfaces

None changed. check-migrations guards the SQLite surface; its hashes
agree byte-for-byte with the existing Rust shipped_migrations_are_immutable
pins, which is a deliberate cross-check rather than a coincidence.

Merge style

Merge commit, please — per-commit rationale is the substance here (PR #43 /
#80 precedent). Eight focused commits.

After merge

Branch protection requiring All pre-merge checks — you asked me to set it
up; I'll apply it once CI has gone green once on this PR so the required
check name resolves against a real run.

🤖 Generated with Claude Code


Update: adversarial review + repo settings

An adversarial review of this branch found nine defects, all fixed in
ci: fix the defects an adversarial review of this branch found. Three
would have broken at runtime — the workflow_dispatch artifact name hits
upload-artifact's slash rejection after a 30–60 min build; the feature
issue-form nested validations: illegally and would have been dropped from
the template chooser; and all-green used always(), which turns every
cancelled run into a red X. Four others claimed more than they did — most
importantly check-stories, which enumerated only src/components and so
silently exempted all 33 uncovered components under src/features while
the step name claimed otherwise. It now covers 110 components instead of
54, with those 33 frozen in a baseline that may only shrink.

The first CI run also did its job and failed for a real reason:
Dependency graph was disabled on the repository, so
dependency-review could not run at all. Fixed by enabling it — and,
while there, the free security settings this repo's own threat model
argues for:

Setting Was Now
Dependency graph disabled enabled (1664 packages)
Dependabot security updates disabled enabled
Secret scanning disabled enabled
Secret scanning push protection disabled enabled
Private vulnerability reporting disabled enabled
GitHub Pages disabled enabled (build_type: workflow)

Push protection is the one that earns its place fastest here: it blocks a
commit containing a detected secret before it reaches the remote, and
this repo's worst-case artefact is exactly that — a minisign private key or
a BIP39 phrase. Private vulnerability reporting had to be on for the
channel SECURITY.md and the issue-template contact link both point at;
without it that link 404s.

Update 2: the gates caught this branch

A second review round plus real CI found more, all fixed in
ci: fix the second review round…. The headline: CodeQL failed this PR
with a high-severity alert in the branch's own new script

js/incomplete-sanitization in scripts/check-migrations.ts, where a
filename was escaped with .replace(/\./g, '\\.') before going into a
RegExp. That escapes . but not \. Fixed by not interpolating into a
regex at all.

check-stories was also wrong about this repo's own Storybook layout:
.storybook/main.ts globs src/**/*.stories.*, but the script read only
src/stories/, so a colocated Foo.stories.tsx counted as neither
coverage nor a story — it was reported as an uncovered component. And
src/components was walked non-recursively while src/features was
recursive, so a new src/components/<group>/ was invisible. Both fixed
and regression-tested.

Two gates were also un-clearable or too blunt: the pr-title check never
re-ran on an edited event, so fixing a rejected title did nothing; and
the typos allowlist used repo-wide extend-words, which would have
silenced a stray mis in src/strings.ts — the file the check mainly
exists for. Both tightened.

Finally, four doc claims this branch itself invalidated: DESIGN-SYSTEM.md
rule 4 still said "there is no automated coverage gate", the README said
CodeQL rolls into the aggregator (it's a separate workflow), the PR
template described a pre-commit hook this branch had already changed, and
dependabot.yml claimed dtolnay/rust-toolchain has no version tags.

Branch protection is applied

main now has a ruleset: pull request required, All pre-merge checks
required, plus non_fast_forward and deletion blocks.

One thing needs you: create a fine-grained PAT scoped to this repo with
Contents: read and write, and add it as the RELEASE_PAT secret.
release-prep.yml now uses it to push the bump commit. I verified against
a throwaway branch that (a) required_status_checks blocks direct pushes,
not just merges, (b) GitHub refuses an Actions bypass actor on a
user-owned repo ("must be part of the ruleset source or owner
organization"), and (c) a push authenticated as a repo admin is let
through. The gate job fails fast with that explanation if the secret is
missing, so nothing breaks silently.

Copilot AI review requested due to automatic review settings July 27, 2026 07:30

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@scotej, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 4 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 6b852bb2-be9f-4db8-9c1b-710366803e5e

📥 Commits

Reviewing files that changed from the base of the PR and between 8be81d6 and 49d5521.

📒 Files selected for processing (27)
  • .github/ISSUE_TEMPLATE/bug_report.yml
  • .github/ISSUE_TEMPLATE/config.yml
  • .github/ISSUE_TEMPLATE/feature_request.yml
  • .github/dependabot.yml
  • .github/pull_request_template.md
  • .github/workflows/ci.yml
  • .github/workflows/codeql.yml
  • .github/workflows/maintenance.yml
  • .github/workflows/pages.yml
  • .github/workflows/pr-build.yml
  • .github/workflows/pr-title.yml
  • .github/workflows/release-prep.yml
  • .github/workflows/release.yml
  • .github/zizmor.yml
  • .husky/pre-commit
  • CLAUDE.md
  • DESIGN-SYSTEM.md
  • ISSUES.md
  • README.md
  • SECURITY.md
  • _typos.toml
  • package.json
  • scripts/check-migrations.ts
  • scripts/check-stories.ts
  • src-tauri/Cargo.toml
  • src-tauri/deny.toml
  • src-tauri/src/db/migrations/MANIFEST.sha256
📝 Walkthrough

Walkthrough

Adds repository contribution templates, security and dependency policies, local migration and Storybook guards, expanded CI enforcement, scheduled analysis workflows, GitHub Pages publishing, and labeled pull-request installer builds.

Changes

Repository quality and automation

Layer / File(s) Summary
Contribution and security policies
.github/ISSUE_TEMPLATE/*, .github/pull_request_template.md, SECURITY.md, _typos.toml, .github/dependabot.yml, ISSUES.md
Adds structured issue and pull request forms, security reporting guidance, spell-check configuration, dependency update rules, and a Tauri advisory ledger entry.
Local integrity checks and supply-chain configuration
scripts/*, package.json, .husky/pre-commit, src-tauri/Cargo.toml, src-tauri/deny.toml, src-tauri/src/db/migrations/MANIFEST.sha256, CLAUDE.md, README.md
Adds migration immutability and Storybook coverage checks, wires them into local commands, and configures Rust dependency validation and documentation.
Pre-merge CI enforcement
.github/workflows/ci.yml, .github/zizmor.yml
Adds workflow, supply-chain, hygiene, title, Storybook, migration, version, and aggregate status gates, while extending checkout hardening and advisory lint reporting.
Scheduled analysis and publishing workflows
.github/workflows/codeql.yml, .github/workflows/maintenance.yml, .github/workflows/pages.yml, .github/workflows/release.yml
Adds CodeQL, OSV, relay, Scorecard, and Storybook Pages workflows, plus release-script comments preserving Markdown backticks.
Pull-request installer builds
.github/workflows/pr-build.yml
Builds labeled macOS and Windows installers, disables updater artifacts, uploads short-lived bundles, and documents their unsigned non-release status.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant PullRequest
  participant CI
  participant AllGreen
  PullRequest->>CI: trigger pre-merge workflow
  CI->>CI: run validation and security jobs
  CI-->>AllGreen: return job results
  AllGreen-->>PullRequest: publish aggregate status
Loading

Possibly related PRs

  • scotej/studyvis#75: Overlaps in CI workflow hardening, token permissions, and action pinning.

Suggested reviewers: copilot

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The PR adds pre-merge quality gates and commit-related validation aligned with #102's goal of better commit testing.
Out of Scope Changes check ✅ Passed The added templates, workflows, docs, and security config all support the stated CI and repository-maintenance objectives.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main additions: new pre-merge checks, supply-chain scanning, and deployment-related CI work.
✨ Finishing Touches
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch ci/premerge-quality-gates

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@scotej
scotej force-pushed the ci/premerge-quality-gates branch from 8be81d6 to e656a86 Compare July 27, 2026 07:32
scotej and others added 8 commits July 27, 2026 17:34
The repo already had a pinning discipline written in prose at the top of
ci.yml — third-party actions SHA-pinned, GitHub-owned ones on major tags,
"refresh manually, there is no dependabot config". Both halves are now
machine-readable.

dependabot.yml covers npm, cargo, and github-actions. The cooldown blocks
are the point: these workflows mint unsigned installers that friends run
past Gatekeeper/SmartScreen, so the failure mode worth designing against
is a compromised-then-yanked release being merged the hour it lands.
Waiting a week costs a weekly reviewer nothing. Wire-format and identity
crates (trystero, @noble/*, @scure/*) and tauri's plugin set are excluded
from the routine groups so a compatibility-surface bump gets its own PR.

Dependabot rewrites a SHA pin's trailing version comment only when the
version is the LAST thing in it (dependabot-core #4691 / PR #5951) — noted
in the file, because the convention now has to hold to keep working.

zizmor.yml states the policy as an unpinned-uses rule rather than a
paragraph. Verified locally: with this config the high-severity /
high-confidence pass is clean on the existing three workflows, so it can
gate rather than nag.

Refs #102

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two house rules that CLAUDE.md states as requirements and nothing
enforced end to end.

check-migrations.ts — migrations.rs opens with "never edit a shipped
migration in place", and the Rust test shipped_migrations_are_immutable
does pin the three hashes. This adds a manifest outside that file and two
checks the Rust test does not make: that every NNN_*.sql is actually
registered (include_str! AND a MIGRATIONS tuple — a file missing the
second is inert), and that the version sequence has no gaps. It runs in
the frontend job in a second rather than behind a full Tauri compile on
two runners, so the feedback lands first. Its hashes agree with the Rust
pins byte for byte, CRLF normalisation included.

`--update` deliberately refuses to rewrite an existing hash: adding a
migration is a one-command act, laundering an edit as an addition is not.
Verified against all three failure modes plus that refusal.

check-stories.ts — "Storybook for every component; mandatory" was
unenforced. It matters more here than it sounds: vitest is node-env with
no jsdom and there are no *.test.tsx, so a component without a story has
neither a test nor an axe-core run. Coverage is resolved by import, not
filename (stories are PascalCase, ui/ primitives are kebab-case). Three
genuine gaps are frozen with per-entry reasons; stale entries are
themselves an error, which is how a wrong entry in my first draft got
caught.

Both are sub-second, so both join the pre-commit hook.

Refs #102

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
cargo-deny closes a real hole: `cargo check --locked` proves the lockfile
is honoured, not that anything in it is safe to ship, and the Rust half of
a self-updating unsigned installer had no supply-chain gate at all.

deny.toml is derived from the actual 629-package graph rather than a
template — the license allowlist is exactly the licences present, so a
dependency introducing a new one fails and gets a line with a reason.
`targets` is restricted to the two shipped platforms, which drops the
Linux gtk/glib tree that is in the lockfile but in no shipped binary.

The advisories section is the part worth reading. It is blocking, but the
eight findings that existed the day it landed are handled two ways:
`unmaintained = "workspace"` drops six transitive abandoned crates that no
change to this repo can fix, and the two real quick-xml DoS advisories are
pinned by ID with reasons. Anything NEW still fails — which is the whole
point, and is why this is not simply `continue-on-error`.

Marking the app crate `publish = false` is what lets [licenses.private]
skip its empty license field; it is also just true.

_typos.toml: all 24 initial hits were false positives (unparseable,
NULLable, base64 fixtures, AB-BA lock ordering), each checked against the
source rather than batch-silenced. Verified it still catches real typos
afterwards. It earns a blocking gate mainly for src/strings.ts, where a
typo ships inside the binary and stays until the next release.

Refs #102

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
New jobs beside the two slow ones rather than inside them, so wall-clock
does not grow:

- workflows — actionlint (pinned tarball, checksum verified before
  extraction: this is a binary download into the job that audits the
  release pipeline) and zizmor. zizmor runs twice: blocking at
  high/high, which in practice is the hash-pinning policy and is verified
  clean today, then advisory at pedantic into the Security tab so the
  low-confidence heuristics stay visible without gating merges.
- supply-chain — dependency-review scores only what a PR ADDS, so the
  known backlog does not block unrelated work while a newly introduced
  vulnerability still fails. cargo-deny blocks. npm audit is advisory
  into the job summary because it is red today (I19) and a permanently
  red gate teaches people to merge past red.
- hygiene — typos blocking, link check advisory (link rot is other
  people's sites moving; it must never block a code PR).
- pr-title — conventional-commit shape, since PRs squash into main.

Added to the frontend job: the migration and story guards, the
version-lockstep script (it existed but only ran at release time, i.e.
after release-prep had already pushed the immutable tag), and a Storybook
artifact upload on PRs.

Added to the rust job: a non-blocking clippy pass over suspicious /
complexity / perf. The blocking step allows every group except
correctness, so that backlog has never been seen and cannot be measured
on a box that cannot compile the crate. This measures it into the job
summary; promote groups once they reach zero.

all-green is the single check for branch protection, so adding a job here
extends the gate without editing the protection rule. needs is fed
through env rather than interpolated into the script — inlining
toJSON(needs) is the injection shape zizmor exists to catch, and this file
is what the rest of the repo copies from.

release.yml gets the one real actionlint finding fixed: a shellcheck
disable for the deliberately single-quoted release-notes heredoc.
actionlint and zizmor are both clean across all workflows.

Refs #102

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CodeQL over javascript-typescript, rust, and actions — all build-mode
none, so the Rust leg needs no webkit2gtk/WebView2 prerequisites on the
runner. Advanced setup rather than default setup because default setup
generates and manages its own workflow, and two competing sources of
truth for "what scans this repo" is worse than one longer file.

maintenance.yml holds the checks that answer "has the world moved under
us?" rather than "is this diff good?". They can all go red with no commit
at all, so none of them belong on a pull request:

- OSV over both lockfiles. This is not redundant with cargo-deny: OSV
  reads the GitHub Advisory Database too, and that difference already
  paid for itself — GHSA-7gmj-67g7-phm9 against the pinned tauri is
  GHSA-only and cargo-deny's RustSec feed does not carry it.
- Relay health. release-prep already runs check-relays non-blocking,
  which is the right moment to act on rot but the wrong one to first
  learn about it — a relay that died three weeks ago has been degrading
  discovery for every install since.
- OpenSSF Scorecard, results kept private: useful as a checklist, and
  there is no audience for a badge on a friends-only project.

Refs #102

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two deployments, aimed at the same gap: CI proves the code compiles and
passes the automated gates, and that is genuinely all it proves.

pages.yml publishes the component workbench from main. Storybook is
already mandatory and already built every CI run, but reading it meant
cloning the repo — so the design system was reviewable only by people who
had already checked out the code. Nothing secret goes up: stories render
against fixture props with no Tauri APIs, no database, no identity
material. PR previews ride on ci.yml's existing build as an artifact
rather than a second build here, because Pages has exactly one live site.

pr-build.yml produces real, installable macOS and Windows bundles from a
PR. The bugs this app actually ships — a camera stream that never reaches
a late joiner, a sidecar that dies at session start — are only visible
with two builds talking to each other, and until now the first installable
build of any change appeared after release-prep had already pushed an
immutable tag.

Label-gated, because a full Tauri build is 30-60 minutes per platform and
most PRs here are docs or CI. createUpdaterArtifacts is switched off via a
config override, which is both what lets it run with no signing secrets
and what guarantees no PR artifact can ever be fed to a client's updater.
pull_request, never pull_request_target, and the rust cache is read-only
for these jobs — a PR is untrusted input and must not write the cache the
release pipeline reads.

Refs #102

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The PR template asks the two questions this repo's CI structurally cannot
answer: what was manually exercised (and whether an agent or a human did
it, per CLAUDE.md's desktop-testing note), and which cross-version
compatibility surface the change touches.

SECURITY.md is written for what this project actually is. Most of a
generic policy would be noise here — no server, no accounts, no
telemetry, peers are friends rather than strangers — so it says that
plainly, points at the two ISSUES.md entries that are accepted deviations
rather than unreported bugs, and then names the part that is genuinely
sensitive: the updater's minisign keypair and the release workflows,
where a finding is effectively RCE on every install.

Refs #102

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CLAUDE.md's quality-gates block and the README's Developing section both
listed commands that no longer match what runs; both now do, including
what CI adds on top of the local gates and which single check branch
protection requires.

I78 records GHSA-7gmj-67g7-phm9 (tauri 2.11.0, origin confusion letting
remote pages invoke local-only IPC commands, CVSS 8.8, fixed in 2.11.1).
Found by OSV while building these gates, not by cargo-deny — RustSec does
not carry it. Left open on purpose: a Tauri bump is a Rust change that
cannot be compiled on this box, so it wants its own PR and its own CI run
rather than riding along with CI config. Dependabot will open it, and the
weekly OSV scan keeps reporting until it lands.

Refs #102

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@scotej
scotej force-pushed the ci/premerge-quality-gates branch from e656a86 to 3b4f5be Compare July 27, 2026 07:36
@github-advanced-security

Copy link
Copy Markdown

You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool.

What Enabling Code Scanning Means:

  • The 'Security' tab will display more code scanning analysis results (e.g., for the default branch).
  • Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results.
  • You will be able to see the analysis results for the pull request's branch on this overview once the scans have completed and the checks have passed.

For more information about GitHub Code Scanning, check out the documentation.

Comment thread .github/workflows/ci.yml
2>&1 | tee clippy.log || true
count=$(grep -c 'warning:' clippy.log || true)
{
echo "### clippy backlog — ${{ matrix.label }}"
Comment thread .github/workflows/ci.yml
contents: read
# The advisory pass uploads SARIF so the low-confidence findings live
# in the Security tab instead of reddening a merge.
security-events: write
Comment thread .github/workflows/ci.yml Fixed
permissions:
contents: read
# Writing results into the Security tab is the entire point of the job.
security-events: write
Comment on lines +11 to +15
on:
schedule:
# Monday morning, off the hour to dodge GitHub's cron stampede.
- cron: '23 6 * * 1'
workflow_dispatch:
cache: 'npm'

- name: Install Rust toolchain
uses: dtolnay/rust-toolchain@2c7215f132e9ebf062739d9130488b56d53c060c # master; the toolchain input selects the channel
cache: 'npm'

- name: Install Rust toolchain
uses: dtolnay/rust-toolchain@2c7215f132e9ebf062739d9130488b56d53c060c # master; the toolchain input selects the channel
# (build.rs only writes a placeholder for debug builds).
- name: Fetch llama-server prebuild
shell: bash
run: bash scripts/fetch-llama-server.sh --triple ${{ matrix.llama-triple }}
- name: Build the app bundle (no updater artifacts, no signing)
shell: bash
run: |
npm run tauri -- build ${{ matrix.args }} \
shell: bash
run: |
{
echo "### ${{ matrix.label }} test build"
Comment thread scripts/check-migrations.ts Fixed
coderabbitai[bot]
coderabbitai Bot previously requested changes Jul 27, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (1)
.github/pull_request_template.md (1)

35-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Align the PR checklist with all blocking gates.

The template omits the new workflow/action linting, dependency review, cargo-deny, typo, migration, version-lockstep, Storybook-coverage, and PR-title gates described by this PR. Add them here or state explicitly that they are CI-only; otherwise the checklist does not capture the repository’s complete pre-merge process.

🤖 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/pull_request_template.md around lines 35 - 45, Update the “Gates”
section in .github/pull_request_template.md to include every blocking gate
introduced by the PR: workflow/action linting, dependency review, cargo-deny,
typo checks, migration checks, version-lockstep validation, Storybook coverage,
and PR-title validation. For any gate not intended for local execution,
explicitly mark it as CI-only so the checklist represents the complete pre-merge
process.
🤖 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/pr-build.yml:
- Line 48: Increase the installer job’s timeout-minutes from 60 to a 90–120
minute value so setup, dependency installation, sidecar download, Tauri
packaging, and artifact upload can complete on slow valid runs.
- Around line 69-72: Update the node-version setting in the setup-node step to
Node 24, then update the project’s package.json and lockfile engine metadata to
target Node 24 wherever the project declares a Node version.

In `@scripts/check-migrations.ts`:
- Around line 182-202: Update the --update path in the migration-checking flow
to validate manifest entries that are missing from the migration tree before
calling renderManifest(entries). Reuse the existing orphan-check logic and
failure behavior from the !update branch so deleted migrations are reported and
the manifest is not rewritten, while preserving the current changed-hash
protection and normal additions.

In `@SECURITY.md`:
- Around line 15-19: Update SECURITY.md lines 15-19 to replace the absolute
“Nothing is uploaded, ever” claim with the guarantee that no telemetry, account
data, or server-hosted history is retained, while acknowledging that peer
signaling or traffic may traverse relays. Update
.github/ISSUE_TEMPLATE/feature_request.yml lines 33-40 to revise the
feature-request checkbox so peer features are not required to claim that no
bytes leave the machine.

---

Nitpick comments:
In @.github/pull_request_template.md:
- Around line 35-45: Update the “Gates” section in
.github/pull_request_template.md to include every blocking gate introduced by
the PR: workflow/action linting, dependency review, cargo-deny, typo checks,
migration checks, version-lockstep validation, Storybook coverage, and PR-title
validation. For any gate not intended for local execution, explicitly mark it as
CI-only so the checklist represents the complete pre-merge process.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4174dd91-2119-49b1-bd16-4e9d66790c09

📥 Commits

Reviewing files that changed from the base of the PR and between 59ea18f and 8be81d6.

📒 Files selected for processing (24)
  • .github/ISSUE_TEMPLATE/bug_report.yml
  • .github/ISSUE_TEMPLATE/config.yml
  • .github/ISSUE_TEMPLATE/feature_request.yml
  • .github/dependabot.yml
  • .github/pull_request_template.md
  • .github/workflows/ci.yml
  • .github/workflows/codeql.yml
  • .github/workflows/maintenance.yml
  • .github/workflows/pages.yml
  • .github/workflows/pr-build.yml
  • .github/workflows/release.yml
  • .github/zizmor.yml
  • .husky/pre-commit
  • CLAUDE.md
  • ISSUES.md
  • README.md
  • SECURITY.md
  • _typos.toml
  • package.json
  • scripts/check-migrations.ts
  • scripts/check-stories.ts
  • src-tauri/Cargo.toml
  • src-tauri/deny.toml
  • src-tauri/src/db/migrations/MANIFEST.sha256
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,rs}

📄 CodeRabbit inference engine (CLAUDE.md)

Add comments only when the reason is non-obvious; identifiers should carry meaning and code should read top-to-bottom.

Files:

  • scripts/check-stories.ts
  • scripts/check-migrations.ts
**/*.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

TypeScript must remain strict and pass the project build and type checks.

Files:

  • scripts/check-stories.ts
  • scripts/check-migrations.ts
.github/workflows/*.yml

📄 CodeRabbit inference engine (CLAUDE.md)

GitHub workflows must pass actionlint and shellcheck; third-party actions must be hash-pinned, while GitHub-owned actions may float on a major tag.

Files:

  • .github/workflows/release.yml
  • .github/workflows/pages.yml
  • .github/workflows/codeql.yml
  • .github/workflows/pr-build.yml
  • .github/workflows/maintenance.yml
  • .github/workflows/ci.yml
src-tauri/deny.toml

📄 CodeRabbit inference engine (CLAUDE.md)

Run cargo deny check to enforce supply-chain advisories, licenses, bans, and sources for the Rust backend.

Files:

  • src-tauri/deny.toml
.github/dependabot.yml

📄 CodeRabbit inference engine (CLAUDE.md)

Keep a SHA pin's trailing # vX.Y.Z comment formatted so the version is the last thing in the comment.

Files:

  • .github/dependabot.yml
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: scotej/studyvis

Timestamp: 2026-07-27T07:31:35.642Z
Learning: Read the relevant canonical documents (`PLAN.md`, `ARCHITECTURE.md`, and `DESIGN-SYSTEM.md`) before non-trivial work; they are the source of truth.
Learnt from: CR
Repo: scotej/studyvis

Timestamp: 2026-07-27T07:31:35.642Z
Learning: Surface conflicts with `PLAN.md`, `ARCHITECTURE.md`, or `DESIGN-SYSTEM.md` instead of silently deviating.
Learnt from: CR
Repo: scotej/studyvis

Timestamp: 2026-07-27T07:31:35.642Z
Learning: Do not add new documentation files unless asked; update canonical documentation, `CHANGELOG.md`, or `ISSUES.md` only when justified.
Learnt from: CR
Repo: scotej/studyvis

Timestamp: 2026-07-27T07:31:35.642Z
Learning: Maintain scope discipline: do not refactor adjacent code during feature work or add abstractions for hypothetical future needs.
Learnt from: CR
Repo: scotej/studyvis

Timestamp: 2026-07-27T07:31:35.642Z
Learning: Use one focused change per commit, with a Conventional Commit subject (`feat:`, `fix:`, `chore:`, `docs:`, or `ci:`); pull requests are squash-merged.
Learnt from: CR
Repo: scotej/studyvis

Timestamp: 2026-07-27T07:31:35.642Z
Learning: Before committing or opening a pull request, run the documented quality gates, including build, lint, tests, token/string/migration/story/contrast/accessibility checks, and applicable Rust checks.
Learnt from: CR
Repo: scotej/studyvis

Timestamp: 2026-07-27T07:31:35.642Z
Learning: Use Context7 rather than web search for library, API, and framework documentation, and verify load-bearing external-library facts before relying on them.
Learnt from: CR
Repo: scotej/studyvis

Timestamp: 2026-07-27T07:31:35.642Z
Learning: Use desktop control tooling when useful for live-app verification; confirm before destructive on-screen actions and document machine-walked versus user-walked testing.
🪛 ast-grep (0.44.1)
scripts/check-migrations.ts

[warning] 135-140: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
const\\s+(\\w+)\\s*:\\s*&str\\s*=\\s*include_str!\\("migrations/${entry.file.replace( /\./g, '\\.' )}"\\)
)
Note: [CWE-1333] Inefficient Regular Expression Complexity

(regexp-from-variable)


[warning] 143-145: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
\\(\\s*${entry.version}\\s*,\\s*${constName}\\s*\\)
)
Note: [CWE-1333] Inefficient Regular Expression Complexity

(regexp-from-variable)

🪛 LanguageTool
SECURITY.md

[uncategorized] ~36-~36: The official name of this software platform is spelled with a capital “H”.
Context: .... The release workflows themselves. .github/workflows/release.yml and `release-...

(GITHUB)

CLAUDE.md

[uncategorized] ~84-~84: The official name of this software platform is spelled with a capital “H”.
Context: ...ull requests) Everything above runs in .github/workflows/ci.yml, plus checks that onl...

(GITHUB)


[uncategorized] ~86-~86: The official name of this software platform is spelled with a capital “H”.
Context: ...ver inline run: blocks) and zizmor. .github/zizmor.yml encodes the pinning policy:...

(GITHUB)


[uncategorized] ~94-~94: The official name of this software platform is spelled with a capital “H”.
Context: ...an update. Dependency bumps arrive via .github/dependabot.yml (npm, cargo, actions). ...

(GITHUB)

🪛 OpenGrep (1.25.0)
scripts/check-migrations.ts

[ERROR] 51-51: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.

(coderabbit.command-injection.exec-js)

🔇 Additional comments (27)
.github/workflows/codeql.yml (1)

1-84: LGTM!

.github/workflows/maintenance.yml (1)

1-108: LGTM!

.github/workflows/pages.yml (1)

1-85: LGTM!

.github/workflows/release.yml (1)

75-78: LGTM!

.github/ISSUE_TEMPLATE/bug_report.yml (1)

1-92: LGTM!

.github/ISSUE_TEMPLATE/config.yml (1)

1-9: LGTM!

_typos.toml (1)

1-49: LGTM!

.github/dependabot.yml (1)

1-127: LGTM!

ISSUES.md (1)

90-90: LGTM!

.github/workflows/pr-build.yml (2)

1-47: LGTM!

Also applies to: 49-68, 74-92, 117-137


93-116: 📐 Maintainability & Code Quality

Workflow lint passes.

scripts/check-stories.ts (1)

1-125: LGTM!

package.json (1)

23-25: LGTM!

.husky/pre-commit (1)

4-5: LGTM!

src-tauri/src/db/migrations/MANIFEST.sha256 (1)

1-10: LGTM!

src-tauri/Cargo.toml (1)

10-14: LGTM!

src-tauri/deny.toml (2)

1-39: LGTM!

Also applies to: 48-107


40-47: 🔒 Security & Privacy

No change needed. The ignored IDs are valid quick-xml DoS advisories patched in 0.41.0, consistent with the comment.

CLAUDE.md (2)

72-94: LGTM!


132-132: LGTM!

README.md (1)

349-370: LGTM!

.github/workflows/ci.yml (5)

110-116: 🎯 Functional Correctness

Script referenced but not in this review batch.

scripts/check-version-lockstep.sh isn't among the files reviewed here — please confirm it exists, accepts a single version-string argument, and validates all five tracked-version files described in CLAUDE.md's Releases section.


142-142: 📐 Maintainability & Code Quality

dtolnay/rust-toolchain pin's trailing comment doesn't end with a version, unlike every other pin in this file.

Every other third-party uses: in this workflow ends its trailing comment with a bare # vX.Y.Z (e.g. # v2.9.1, # v0.6.1, # v5.0.0). This one is # master; the toolchain input selects the channel — not a version at all, with trailing prose after it. Since the repo's stated convention (and the coding guideline for .github/dependabot.yml) is that Dependabot rewrites a SHA pin's comment only when the version is the last token, please confirm whether this pin is intentionally exempt from Dependabot's automatic refresh (similar to the documented actionlint exception in the workflows job) or whether the comment should be reformatted so tooling can manage it.

As per coding guidelines, .github/dependabot.yml: "Keep a SHA pin's trailing # vX.Y.Z comment formatted so the version is the last thing in the comment."

Source: Coding guidelines


31-116: LGTM!

Also applies to: 117-206, 207-289, 290-351, 352-391, 434-468


241-253: 🔒 Security & Privacy

No change needed. The actionlint 1.7.12 linux_amd64 release checksum matches the pinned value.


392-433: 🩺 Stability & Availability

No change needed. This job only validates the PR title and does not use the action’s status/comment features, so pull-requests: read is the appropriate minimum here.

.github/zizmor.yml (1)

1-31: LGTM!

Comment thread .github/workflows/pr-build.yml Outdated
Comment on lines +69 to +72
- uses: actions/setup-node@v6
with:
node-version: '20'
cache: 'npm'

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

while IFS= read -r file; do
  printf '== %s ==\n' "$file"
  rg -n 'node-version|engines|20|22|24' "$file" || true
done < <(fd -HI -t f '^(package\.json|package-lock\.json|\.nvmrc|\.node-version|\.tool-versions)$')

Repository: scotej/studyvis

Length of output: 205


🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

printf 'Workflow files:\n'
git ls-files '.github/workflows/*.yml' '.github/workflows/*.yaml' || true

printf '\nTarget workflow excerpt:\n'
if [ -f .github/workflows/pr-build.yml ]; then
  nl -ba .github/workflows/pr-build.yml | sed -n '1,120p'
else
  echo 'MISSING .github/workflows/pr-build.yml'
fi

printf '\nCandidate version declarations:\n'
for f in package.json package-lock.json .npmrc .node-version .nvmrc .tool-versions; do
  if [ -f "$f" ]; then
    printf '== %s ==\n' "$f"
    rg -n '"(engines|engineStrict|packageManager)"|node-version|node:|nodejs|20|22|24|pnpm|npm|yarn' "$f" || true
  fi
done

printf '\nAll tracked files with common version indicators:\n'
rg -n --hidden --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**' '"(engines|packageManager)"|\.node-version|\.nvmrc|node-version|node:|nodejs|20|22|24|pnpm|npm|yarn|^packageManager' . || true

printf '\nNode 20 availability from npm registry (metadata availability check):\n'
node - <<'JS'
const https = require('https');
https.get('https://registry.npmjs.org/node', { timeout: 10000, headers: { 'accept': 'application/json' } }, (res) => {
  let chunks = '';
  res.on('data', (chunk) => chunks += chunk);
  res.on('end', () => {
    try {
      const data = JSON.parse(chunks);
      console.log(data.time && data.time['20.11.1'] ? 'npm metadata includes node 20.11.1' : 'npm metadata missing node 20.11.1');
      console.log('latest:', data['dist-tags']?.latest);
    } catch (err) {
      console.log('npm metadata parse error');
    }
  });
}).on('error', (err) => console.log('registry request error:', err.message));
JS

Repository: scotej/studyvis

Length of output: 447


🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

printf 'Target workflow excerpt:\n'
awk '{printf "%5d\t%s\n", NR, $0}' .github/workflows/pr-build.yml | sed -n '1,120p' || true

printf '\nCandidate version declarations:\n'
for f in package.json package-lock.json .npmrc .node-version .nvmrc .tool-versions; do
  if [ -f "$f" ]; then
    echo "== $f =="
    grep -En '"(engines|engineStrict|packageManager)"|node-version|node:|nodejs|20|22|24|pnpm|npm|yarn' "$f" || true
  fi
done

printf '\nAll tracked files with common version indicators:\n'
grep -RIn --hidden --exclude-dir=node_modules --exclude-dir=dist --exclude-dir=build -E '"(engines|packageManager)"|\.node-version|\.nvmrc|node-version|node:|nodejs|20|22|24|pnpm|npm|yarn|^packageManager' . || true

printf '\nNode 20 availability from npm registry (metadata availability check):\n'
node - <<'JS'
const https = require('https');
https.get('https://registry.npmjs.org/node', { timeout: 10000, headers: { 'accept': 'application/json' } }, (res) => {
  let chunks = '';
  res.on('data', (chunk) => chunks += chunk);
  res.on('end', () => {
    try {
      const data = JSON.parse(chunks);
      console.log(data.time && data.time['20.11.1'] ? 'npm metadata includes node 20.11.1' : 'npm metadata missing node 20.11.1');
      console.log('latest:', data['dist-tags']?.latest);
    } catch (err) {
      console.log('npm metadata parse error');
    }
  });
}).on('error', (err) => console.log('registry request error:', err.message));
JS

Repository: scotej/studyvis

Length of output: 50372


🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

printf 'Focused Node 20 references:\n'
grep -RIn --hidden --exclude-dir=node_modules --exclude-dir=dist --exclude-dir=build --exclude-dir=.git -E '"engines"[[:space:]]*:|packageManager[[:space:]]*:|node-version|node:|nodejs|Node\.js|Node\.js|Node.js' . \
  | grep -Ei '20|22|24|lts|previous|versions|previous-releases' \
  | head -n 80 || true

printf '\nNode package.json fields:\n'
if [ -f package.json ]; then
  python3 - <<'PY'
import json, pathlib, re
p = pathlib.Path('package.json')
data = json.loads(p.read_text())
for key in ('engines','packageManager','scripts','dependencies','devDependencies'):
    if key in data:
        print(key, json.dumps(data[key], indent=2, sort_keys=True)[:2000])
PY
fi

printf '\nTarget lockfile package.json metadata for root package:\n'
python3 - <<'PY'
import json
from pathlib import Path
data = json.loads(Path('package-lock.json').read_text())
root = data.get('packages', {}).get('', {})
print(json.dumps({k: root.get(k) for k in ('name','version','engines','packageManager') if k in root}, indent=2))
PY

Repository: scotej/studyvis

Length of output: 3191


🌐 Web query:

Node.js Release Schedule Node.js 20 EOL March 2026 Node.js 24 current LTS

💡 Result:

As of July 27, 2026, the status of Node.js 20 and 24 is as follows: Node.js 20 reached its End-of-Life (EOL) on April 30, 2026 [1]. It is no longer supported and does not receive security updates [1][2]. Node.js 24 is currently in its Active Long Term Support (LTS) phase [3][4], having entered this phase on October 28, 2025 [3][5]. It is scheduled to remain in the Active LTS phase until October 20, 2026, after which it will transition to Maintenance LTS mode [3][4]. The final End-of-Life date for Node.js 24 is April 30, 2028 [3][4]. For reference, the Node.js project is evolving its release schedule [6]. While the current even-numbered release lines follow the traditional 12-month Active LTS and 18-month Maintenance LTS model, future major releases (beginning with Node.js 27) will transition to a new schedule featuring one major release per year, with each release receiving 30 months of Long Term Support [6][7].

Citations:


Security Misconfiguration (CWE-1104)

Reachability: Internal

Move the workflow off EOL Node 20.

Node.js 20 reached end-of-life on 2026-04-30 and no longer receives security updates, while Node 24 is a supported LTS. Update node-version in .github/workflows/pr-build.yml accordingly, and declare the updated Node version in package.json/lockfile metadata if this project targets Node 24.

🤖 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/pr-build.yml around lines 69 - 72, Update the node-version
setting in the setup-node step to Node 24, then update the project’s
package.json and lockfile engine metadata to target Node 24 wherever the project
declares a Node version.

Comment thread scripts/check-migrations.ts
Comment thread SECURITY.md
Comment on lines +15 to +19
- **All data is local.** Sessions, stats, and the audit log live in a
SQLite database on your own disk. Private keys live in the OS keychain
(macOS Keychain, Windows Credential Manager). Nothing is uploaded, ever.
- **AI inference is on-device.** The llama-server sidecar runs locally;
screenshots and camera frames never leave the machine.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Security Misconfiguration (CWE-16)

Reachability: External

Reconcile the local-data guarantee with relay-carried presence.

The two files describe “nothing leaves the machine,” but ISSUES.md I74 documents ephemeral presence data traversing relays. Preserve the accurate guarantee—no telemetry, account, or server-hosted history—without claiming that peer signaling never leaves the device.

  • SECURITY.md#L15-L19: replace the absolute “Nothing is uploaded, ever” statement with the durable-data/no-telemetry guarantee and mention relay-carried peer traffic.
  • .github/ISSUE_TEMPLATE/feature_request.yml#L33-L40: rewrite the checkbox so valid peer features are not required to claim that no bytes leave the machine.
📍 Affects 2 files
  • SECURITY.md#L15-L19 (this comment)
  • .github/ISSUE_TEMPLATE/feature_request.yml#L33-L40
🤖 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 `@SECURITY.md` around lines 15 - 19, Update SECURITY.md lines 15-19 to replace
the absolute “Nothing is uploaded, ever” claim with the guarantee that no
telemetry, account data, or server-hosted history is retained, while
acknowledging that peer signaling or traffic may traverse relays. Update
.github/ISSUE_TEMPLATE/feature_request.yml lines 33-40 to revise the
feature-request checkbox so peer features are not required to claim that no
bytes leave the machine.

Nine findings, verified individually rather than taken on trust.

Would have broken at runtime:

- pr-build.yml's artifact name fell back to `github.ref_name` on
  workflow_dispatch, and upload-artifact rejects names containing "/".
  Every branch here is namespaced, so the documented dispatch path would
  have built for 30-60 minutes per platform and then died on the upload.
  Uses `github.run_id` instead.
- feature_request.yml nested `validations:` inside `attributes:`, which is
  not a legal key there — GitHub would have rejected the form and dropped
  it from the template chooser. Both templates now validate against the
  schema.
- all-green used `if: always()`, which is true even for a CANCELLED run.
  With cancel-in-progress on, every superseded push would have spun the
  aggregator up, read "cancelled" from each need, and reported a hard
  failure for a run nobody was waiting on. `!cancelled()`.

Claimed more than it did:

- check-stories enumerated only src/components, so all 33 uncovered
  components under src/features were silently exempt while the CI step
  claimed "every component has a story" and the header quoted CLAUDE.md's
  "every primitive and feature component". Now walks features/ too (54
  components -> 110) with those 33 frozen in an explicit baseline that may
  only shrink. Verified it now catches a new feature component.
- The dependency-review step's comment described licence handling that was
  never configured; it scores vulnerabilities only, and now says so.
- pr-build's `save-if: false` comment asserted a cache-poisoning path that
  GitHub's per-ref cache scoping already prevents, and its fork-safety note
  ignored that `synchronize` rebuilds a labelled PR without further review.
  Both rewritten to say what is actually true.
- CLAUDE.md claimed a new ci.yml job is covered by the aggregator
  automatically. `needs:` is a hand-maintained list; the branch-protection
  rule is the part that stops needing edits.
- zizmor.yml pointed at a workflow-lint.yml that was never created.

Hardening the review was right to ask for:

- release.yml's `publish` job holds `contents: write` and was the only
  checkout still persisting its credential into .git/config for `npm ci`
  and a third-party build action to read. It never uses git directly.
  release-prep's `gate` job likewise; its `prep` job keeps the credential
  because it genuinely pushes.
- Dropped `id-token: write` from the Scorecard job, which only needs it
  for `publish_results: true`, and publishing is deliberately off.

Refs #102

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
timeout-minutes: 15
permissions:
contents: read
security-events: write
scotej and others added 3 commits July 27, 2026 18:02
…apply

main is now protected by a ruleset requiring a pull request and the
"All pre-merge checks" status check. That breaks release-prep, and the
usual fix is not available here.

Verified against a throwaway branch rather than assumed, because the docs
are ambiguous on both points:

  - `required_status_checks` blocks DIRECT PUSHES, not just merges. A push
    to a branch carrying only that rule is rejected with
    'Required status check "All pre-merge checks" is expected.'
  - Integration bypass — which would let GITHUB_TOKEN through — is refused
    on a user-owned repository: "Actor GitHub Actions integration must be
    part of the ruleset source or owner organization". This repo has no
    organization.
  - A push authenticated as a repository admin IS let through, with
    "Bypassed rule violations", when the ruleset lists RepositoryRole 5.

So the prep job checks out with RELEASE_PAT (a fine-grained,
this-repo-only, Contents: read+write token owned by the maintainer)
instead of GITHUB_TOKEN. It is the single checkout in the repo that keeps
persist-credentials on, because it is the only one that pushes.

The gate job asserts the secret exists before anything is bumped —
mirroring release.yml's signing-key preflight. Without it the run would
do the whole bump and then fail on `git push` with a rules violation that
reads like a protection misconfiguration rather than a missing secret.

Refs #102

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…script

The pre-merge gates caught the branch that adds them, which is the outcome
worth having.

CodeQL failed PR #105 with one high-severity alert — js/incomplete-sanitization
in scripts/check-migrations.ts, where a filename was escaped with
`.replace(/\./g, '\\.')` before being interpolated into a RegExp: that
escapes `.` but not `\`. Fixed by not interpolating at all — find the
declaration line by substring, then read the const name off it with a
fixed pattern.

check-stories was wrong about this repo's own Storybook layout in two ways.
.storybook/main.ts globs `src/**/*.stories.*`, but the script read only
src/stories/, so a colocated Foo.stories.tsx counted as neither coverage
nor a story — it was reported as an uncovered component. And
src/components was walked non-recursively while src/features was
recursive, so any new src/components/<group>/ was invisible. Both fixed
and both regression-tested.

`check-migrations -- --update` printed a success line and exited 1 with the
violations never shown, so a mis-wired new migration looked like a broken
tool. It now prints them.

The pr-title gate could not be cleared: `on: pull_request` defaults to
opened/synchronize/reopened, and editing a title is an `edited` event — so
fixing a rejected title did not re-run the check, and a title changed after
checks went green was never re-validated. Added `edited`. Also added
`style` to the allowed types; specifying `types` replaces the action's
defaults, and main already carries a `style(backend):` commit.

The typos allowlist leaned on `extend-words`, which silences a correction
repo-wide — `mis = "mis"` to excuse "mis-decoding" would also let a stray
"mis" through in src/strings.ts, the file the check mainly exists for.
Replaced with scoped `extend-ignore-re` patterns; verified the repo is
still clean and that mis/lable/PN/Iz are now caught in prose again.

Doc claims this branch had made false: DESIGN-SYSTEM.md rule 4 still said
"there is no automated coverage gate", README said CodeQL rolls up into
All pre-merge checks (it is a separate workflow), the all-green comment
block contradicted itself about `needs:` after the last round's fix, the PR
template listed a pre-commit hook this branch had already changed, and
dependabot.yml claimed dtolnay/rust-toolchain has no version tags (it
publishes v1; the pin is deliberately a master SHA).

Refs #102

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Observed on this PR, not theorised: with everything green, GitHub still
reported mergeStateStatus BLOCKED.

Cause was the previous commit's own fix. Adding `edited` to ci.yml's
`pull_request` types made the title check clearable — a PR title is edited
outside opened/synchronize/reopened, so without it a rejected title could
never be corrected — but it also meant every description edit re-ran the
entire suite against an UNCHANGED commit. With cancel-in-progress, the
superseded run left a `cancelled` "All pre-merge checks" beside the
successful one on the same SHA, and the required check would not clear:

  $ gh api .../commits/<sha>/check-runs?check_name=All%20pre-merge%20checks
  08:10:49Z  completed/cancelled
  08:16:24Z  completed/success
  $ gh api .../pulls/105 --jq .mergeable_state
  blocked

So the cheap check now listens for `edited` and the expensive suite does
not. ci.yml goes back to the default activity types, `pr-title` moves to
pr-title.yml, and all-green's `needs:` drops it.

Branch protection now requires two checks instead of one. That is the
trade: the aggregator still means adding a job to ci.yml never touches the
protection rule, and the title check gets to re-run on the only event that
can change what it validates.

Refs #102

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
timeout-minutes: 5
permissions:
contents: read
pull-requests: read
SECURITY.md said "Nothing is uploaded, ever". That is false since I74, and
being false in the security policy is the worst place for it: presence now
has a relay-carried leg that publishes an ephemeral Nostr event (kind
20001) to the pinned relays every 30 s, and peer discovery has always ridden
third-party relays. Rewritten to keep the guarantee that is actually true —
no account, no server-side history, no telemetry, no content — while saying
plainly that sealed presence beacons and discovery traffic do leave the
machine, and that traffic-analysis inferences from them are in scope for a
report.

check-migrations' `--update` could silently drop a deleted migration. The
orphan check ("in the manifest, missing from the tree") only ran in the
non-update branch, and the sequence check does not cover it: delete the
HIGHEST-numbered migration and 1..N stays contiguous, so the hash would
have vanished from the manifest with nothing said — the exact case the
script exists to catch, defeated in its own update path. Verified against
that scenario: it now refuses and leaves the manifest untouched.

pr-build's 60-minute timeout contradicted its own header, which says a
build can take up to 60 minutes per platform on its own — before `npm ci`
and the sidecar fetch. Raised to 90; the label already gates the cost and
public-repo minutes are free.

Also listed the migration/story/cargo-deny gates in the PR template and
named the CI-only ones, so the checklist matches the real pre-merge set.

Not taken: bumping this workflow's setup-node to 24. Every other workflow
pins '20' and README's Developing section states Node 20.19+ as the floor;
making one workflow disagree is worse than the thing it would fix, and a
Node bump is its own change.

Refs #102

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@scotej

scotej commented Jul 27, 2026

Copy link
Copy Markdown
Owner Author

Addressed in 49d5521 — three of the four were real:

  • SECURITY.md vs I74 — correct and the most important of the four. "Nothing is uploaded, ever" is false since relay-carried presence landed: an ephemeral kind-20001 Nostr event goes to the pinned relays every 30 s. Rewritten to keep the guarantee that is true (no account, no server-side history, no telemetry, no content) and state plainly that sealed presence beacons and discovery traffic do leave the machine — with traffic-analysis inferences explicitly in scope for a report.
  • --update dropping a deleted migration — confirmed, including your reasoning that the sequence check does not cover it because deleting the highest-numbered migration keeps 1..N contiguous. Verified against exactly that scenario: it now refuses and leaves the manifest untouched.
  • pr-build timeout — fair, and it contradicted the file's own header. Raised to 90.

Not taken: setup-node to Node 24. Every other workflow here pins '20', and README's Developing section states Node 20.19+ as the floor. Making one workflow disagree with the rest of CI is worse than the inconsistency it would resolve, and a Node bump is its own change with its own testing — not something to land inside a CI-configuration PR.

@scotej
scotej dismissed coderabbitai[bot]’s stale review July 27, 2026 08:52

All three actionable findings fixed in 49d5521 (SECURITY.md relay-presence accuracy, --update orphan guard, pr-build timeout). The fourth (setup-node 24) declined with reasoning in a PR comment — the repo pins Node 20 everywhere. Dismissing as addressed.

@scotej
scotej merged commit 6aa025e into main Jul 27, 2026
15 checks passed
@scotej
scotej deleted the ci/premerge-quality-gates branch July 27, 2026 09:04
@coderabbitai coderabbitai Bot mentioned this pull request Jul 28, 2026
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

better commit testing

3 participants