Skip to content

ci: the two sets that need the sentence model run on every push - #232

Closed
znat wants to merge 3 commits into
mainfrom
feat/ci-fetches-the-sentence-model
Closed

znat wants to merge 3 commits into
mainfrom
feat/ci-fetches-the-sentence-model

Conversation

@znat

@znat znat commented Aug 30, 2026

Copy link
Copy Markdown
Owner

Two scored sets were hand-run because they need the 300 MB ModernBERT model:
scripts/check-slot-gate.sh and scripts/check-sentence-join.sh. They are the
only defence of the claim that the app does not rewrite text when nothing asked
it to — which name proposals get written unasked, and which periods get removed
unasked. Out of CI they drift silently. CI now fetches the model and runs both.

make test fetches it too. On a machine that has never run it that is a one-off
40s download plus a 7s Core ML compile; after that the run is a few seconds
slower and nothing else. This is stated here rather than hidden: make test now
touches the network on a fresh clone.

Review first: the cache key on checks.yml, and the new guard at the bottom of
check-slot-gate.sh.

Based on #230. The diff against main will shrink once that merges.

check-slot-gate.sh passed with the model never running — the whole point of it

The script ended with:

[ "$wrong_apply" -eq 0 ] && [ "$wrong_decline" -eq 0 ]

With no cached model every case routes to judge. Nothing is applied, nothing
is declined, both counters are zero, and the script exits 0. Confirmed by
moving the cache aside:

  50 scored, 9 not (tests/judge-cases.yaml)
  applied 0   declined 0   judge 50
  wrong applies 0   wrong declines 0
  exit code: 0

A human reading applied 0 declined 0 off the terminal sees it. CI does not.
A failed fetch or an empty cache restore would have produced a green tick over
nothing.

The fix: fail when applied + declined is zero. The other option was
failing when judge equals the scored total. Both fire on exactly the same
runs today. applied + declined == 0 states the invariant directly — the gate
settled nothing — and it does not need to know how many cases were scored, so
it survives a change to the case set.

  applied 0   declined 0   judge 50
  wrong applies 0   wrong declines 0
  ✗ the gate settled nothing — is the sentence model cached?
    fetch it with: .build/release/ParrotFlow --sentence-model
  exit code: 1
check-sentence-join.sh needs no second guard — it already fails first

Confirmed the same way, cache moved aside:

  ✗ the measured boundary was not found in: if it's because the release notes
    are not bigger than the height. Another features. If there is more than…
  exit code: 1

It fails earlier than expected. The stored per-case score would catch a model
that never ran — drifted compares each case against the number the probe
measured when the set was built, and a tier count with no scores behind it
cannot match. But SentenceJoinCommand guards on SentenceModel.isCached and
prints no boundary block at all, so reading() returns nothing and the run
stops on the first case. Two ways to fail, both before any number is reported.
No guard added.

The numbers CI produces, and why slot-gate is 15/14/21 and not 13/16/21

Measured on this branch, Apple M-series:

check-slot-gate.sh
  50 scored, 9 not (tests/judge-cases.yaml)
  applied 15   declined 14   judge 21
  wrong applies 0   wrong declines 0

check-sentence-join.sh
  cuts          10 joined,  3 offered,  7 left alone  (of 20)
  real periods   0 joined,  0 offered, 20 left alone  (of 20)
  join tier      50% of cuts repaired, 0.0 false joins per 100 real periods
  offer tier     15% more cuts caught, 0.0 per 100 real periods offered

15/14/21 and not 13/16/21. The split moved in #229, which added the sentence to
the slot decision; SlotGate.swift:46 and docs/transcription.md already record
15/14/21. The judge count and the settled total are the same either way, and
both error counts are still zero.

Neither script asserts a split. check-slot-gate.sh asserts zero wrong applies
and zero wrong declines, plus the new "settled something". check-sentence-join.sh
asserts zero false joins and zero drift. That matters on a runner: a small shift
in the split is not a failure, but a wrong apply is.

Run time — 4 min today, and where the new work lands against the 20 min limit

Measured locally, warm:

step cost
--sentence-model, cached 5.3s
--sentence-model, cold ~40s download + 7s compile
check-slot-gate.sh 20s
check-sentence-join.sh 11s

So about 36s warm and 78s cold on top of the current ~4 min run. make test
went from about 1:20 to 1:53 on a warm build.

The real CI numbers are in the checks on this PR — the first run is the cold
one, any push after it is warm. timeout-minutes stays at 20. I have not
raised it, and if the cold run comes anywhere near it that is worth saying out
loud rather than papering over.

The warm fetch is 5.3s, not the ~0.1s a no-op would cost. It loads the compiled
model into Core ML as well as checking the cache, which is the same call the
first English dictation makes.

What the cache is keyed on, and why not a revision sha
path: "~/Library/Application Support/ParrotFlow/models"
key: sentence-model-${{ runner.os }}-${{ hashFiles('Sources/ParrotFlow/SentenceModel.swift') }}
restore-keys: sentence-model-${{ runner.os }}-

SentenceModel.swift holds the repository, the file list and the package name.
Change any of them and the cached copy is the wrong one.

HubDownload pins to the HuggingFace repository's main
(HubDownload.swift:36), not to a revision sha, so there is no upstream
revision to key on. The restore-keys fallback takes the last run's copy.

The path is ParrotFlow and not ParrotFlow Dev. AppVariant.displayName
reads the bundle identifier and a bare SwiftPM binary has none, so
AppVariant.isDev is false.

Comments that said the opposite, and one list that was already out of step

Updated, because a comment claiming a check does not run is worse than none:

  • scripts/check-slot-gate.sh header — "Not in make test and not in CI".
  • scripts/check-sentence-join.sh header — the same, plus the claim that a
    missing model just makes the run "say so".
  • .github/workflows/checks.yml top block — "without a model" became "without
    an LLM", and "Four of the nine scripts/check-*.sh" lost its count. There
    are 41 of them now, so the number was long stale.
  • The sentence-case step comment on the base branch, which says the scored
    half "is run by hand".
  • Makefile — the CHECKS comment.
  • CONTRIBUTING.md, docs/cli.md (three places), docs/transcription.md.

Not fixed here: CHECKS and checks.yml were already out of step before
this PR. keyed is in the Makefile and not in CI; bug-report is in CI and not
in the Makefile. Both predate this change and neither is about the sentence
model, so they are left alone rather than folded into a CI change.

znat and others added 3 commits August 30, 2026 03:17
Stop for breath mid-sentence and the transcriber writes a period, then
capitalises the next word. "you should see a parrot at the top right of
your screen" comes out as two sentences.

`SentenceProbe` has answered this question since #227 and nothing called
it. This is the consumer. After the vocabulary pass, every
`word. Capital` boundary in an English transcript is scored, and two
thresholds decide: below -4 the period is removed silently, below -2 the
join is recorded and not written, above that nothing happens. Both are
`transcription.sentences` in the config, and `sentences: false` turns the
stage off.

Joining lowercases the word after the period, unless there is a reason
not to: `I`, a word in capitals throughout, a name NLTagger recognises, a
word NLTagger gives no lemma for, or a term in vocabulary.yaml.

English only, and only where the sentence model is already on disk.
Nothing is downloaded and nothing waits.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NMVgtb4jmhQtvWtSJr1bce
Signed-off-by: Nathan Zylbersztejn <nathan.zylbersztejn@gmail.com>
Signed-off-by: Nathan Zylbersztejn <nathan.zylbersztejn@gmail.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`check-slot-gate.sh` and `check-sentence-join.sh` were hand-run because they
need the 300 MB ModernBERT model. They are the only defence of the claim that
the app does not rewrite text when nothing asked it to, so out of CI they
drift silently.

CI now fetches the model with `--sentence-model` after the build and caches it
at `~/Library/Application Support/ParrotFlow/models`, keyed on
`SentenceModel.swift`. Both scripts are added to `checks.yml` and to `CHECKS`
in the Makefile, and `make test` fetches the model too.

`check-slot-gate.sh` passed vacuously without a model: every case routed to
`judge`, so nothing was applied, nothing was declined, both wrong counters
were zero and the script exited 0. It now fails when `applied + declined` is
zero.

Signed-off-by: Nathan Zylbersztejn <nathan.zylbersztejn@gmail.com>
@greptile-apps

greptile-apps Bot commented Aug 30, 2026

Copy link
Copy Markdown

Greptile Summary

This change adds sentence-model caching and runs model-backed regression checks in CI. The cache fallback can reuse an older but loadable model without checking its upstream revision or expected content, so the scored checks may run against stale weights. The workflow also fetches the cache action through a mutable tag, allowing the executed CI action code to change without a repository change. These issues should be fixed before merging.

Confidence Score: 2/5

The change is not safe to merge until model-cache freshness is enforced and the third-party action is immutably pinned.

Two independent failures were reproduced: an older complete sentence-model cache skips the fetch path, and a mutable action tag resolves to different code after the tag is moved.

Files Needing Attention: .github/workflows/checks.yml needs an immutable actions/cache reference and a model cache strategy keyed to a pinned model revision; Sources/ParrotFlow/SentenceModel.swift needs cache freshness validation before accepting local artifacts.

Security Review

The new actions/cache@v4 reference uses a mutable tag for executable CI code. A tag move can make the unchanged workflow run replacement action content, creating a CI/CD supply-chain integrity risk. Pin the action to a reviewed full commit SHA.

T-Rex T-Rex Logs

What T-Rex did

  • Ran the executable stale-sentence-cache check and observed behavior for empty versus loadable caches, with a Linux runtime blocker showing swift: not found.
  • Reviewed the targeted immutable action-reference checker source and observed CI tag moves before and after a forced tag move.
  • Validated the exact stale-sentence-cache flow and its before/after states, including the isCached decisions and the Linux runtime blocker that led to using a static check.
  • Validated workflow pinning integrity for actions in checks.yml, including before/after repro and guidance to pin actions with a full commit SHA.

View all artifacts

T-Rex Ran code and verified through T-Rex

Comments Outside Diff (2)

  1. General comment

    P1 Prefix-restored stale sentence-model cache bypasses upstream freshness validation

    • Bug
      • The workflow permits restoring any prior sentence-model-${runner.os}- cache at .github/workflows/checks.yml:91-92. The subsequent .build/release/ParrotFlow --sentence-model command at :100-101 treats a cache with tokenizer.json plus the three compiled CoreML files as cached (Sources/ParrotFlow/SentenceModel.swift:77-83). In build at :115-124, a loadable cache is returned immediately and HubDownload.fetch is not reached. That branch does not compare an upstream commit/revision, model manifest/content hash, repository state, or a persisted current-file specification. The executed older-cache scenario therefore accepted stale weights, which the model-backed scored checks subsequently use.
    • Cause
      • The cache restore key intentionally falls back to prior OS-matching cache entries, while the runtime cache validity predicate checks only local file existence and whether CoreML can load the compiled artifact.
    • Fix
      • Pin the Hugging Face model to an immutable revision/content digest and persist/validate a cache manifest containing that revision plus the expected file list and hashes before accepting isCached; on mismatch, remove/refetch/recompile. Include the pinned revision/spec in the GitHub Actions cache key and avoid a fallback restore key that can reintroduce incompatible model artifacts.

    T-Rex Ran code and verified through T-Rex

  2. General comment

    P1 actions/cache is referenced through mutable v4 tag

    • Bug
      • At .github/workflows/checks.yml:86, uses: actions/cache@v4 is not a 40-character commit SHA. The executed checker classified it as MUTABLE_TAG; the before/after local Git reproduction force-moved a v4 tag and showed the identical tag reference resolve from trusted-cache-action to replacement-cache-action.
    • Cause
      • The workflow selects the external action by its movable release tag rather than by an immutable Git object ID.
    • Fix
      • Pin actions/cache to the reviewed full 40-character commit SHA for the desired v4 release, and update that SHA through a trusted review/automation process.

    T-Rex Ran code and verified through T-Rex

Fix all with Greploop Fix All in Claude Code

Reviews (1): Last reviewed commit: "ci: the two sets that need the sentence ..." | Re-trigger Greptile

# quoted only so nobody reads it as two paths.
path: "~/Library/Application Support/ParrotFlow/models"
key: sentence-model-${{ runner.os }}-${{ hashFiles('Sources/ParrotFlow/SentenceModel.swift') }}
restore-keys: sentence-model-${{ runner.os }}-

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Prefix-restored model cache bypasses freshness checks

The sentence-model-${{ runner.os }}- restore prefix can recover an older model cache after the exact key changes. The following --sentence-model invocation accepts a cache containing locally loadable expected files and returns it before downloading; it does not compare an upstream revision, manifest, or content hash. The slot-gate and sentence-join checks can therefore score stale model weights while appearing to use the current model configuration. Pin the model revision, validate a persisted manifest or content hashes before reuse, and include that immutable revision in the cache key.

Artifacts

Executable stale sentence-cache decision check

  • This authored narrow check asserts the production cache predicate and cached-load control flow, then exercises empty and older-cache scenarios; the takeaway is that the exact decision path is testable without the unavailable macOS CoreML runtime.

Empty sentence-model cache takes fetch path

  • The executed before scenario reports `isCached=False` and a fetch decision; the takeaway is that the control checks the cache predicate before fetching.

Older loadable sentence-model cache is accepted

  • The executed after scenario marks the cache `older-upstream-revision`, reports `isCached=True`, and takes `load cached copy; do not call fetch`; the takeaway is that stale loadable weights bypass freshness validation.

Native Swift runtime unavailable in Linux validation runner

  • The attempted `swift --version && swift build -c release` command exits 127 because `swift` is not installed; the takeaway is that a macOS/CoreML end-to-end binary run was blocked by the environment.

View artifacts

T-Rex Ran code and verified through T-Rex

Fix in Claude Code

# AppVariant.displayName, which reads the bundle identifier, and a bare
# SwiftPM binary has none.
- name: Cache the sentence model
uses: actions/cache@v4

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 security Cache action is selected through a mutable tag

actions/cache@v4 selects external executable CI code by a movable tag rather than an immutable commit. If that tag is moved upstream, the same workflow revision can execute replacement code without a repository change. Pin the action to the reviewed full 40-character commit SHA for the intended release and update that SHA through the normal dependency-update process.

Artifacts

Targeted immutable action-reference checker source

  • Captured the authored checker that reads workflow line 86, validates the reference format, and executes a local tag-move reproduction, with the takeaway that the check directly exercises the reported CI integrity condition.

CI action tag before a tag move

  • Executed the checker before moving a local v4 tag; it found `actions/cache@v4` at line 86 and resolved the tag to trusted action content, with the takeaway that the workflow uses a non-SHA tag.

CI action tag after a forced tag move

  • Executed the same checker after force-moving the unchanged local v4 tag; it resolved to replacement action content, with the takeaway is that a tag move can change CI code without changing the workflow.

View artifacts

T-Rex Ran code and verified through T-Rex

Fix in Claude Code

Base automatically changed from feat/join-sentences-a-pause-cut to main August 30, 2026 09:33
@znat znat closed this Sep 15, 2026
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.

1 participant