Repository navigation
Slim agent instructions and reject CJK ideographs in changed files - #71
Conversation
There was a problem hiding this comment.
Approve with nits (3 P3)
Scope: base main, merge base 1843eff, 3 commits (one a merge), 5 files reviewed. Loaded coderbook, ponytail-review and karpathy-guidelines. Ran in a separate session.
Findings
P3
-
The CJK check passes silently when
git greperrors (scripts/ci/check-repository-gate.sh:66)
The check isif ... && git grep ...; then fail; fi.git grepreturns 0 on a match, 1 on no match and 128 on an error. Because theiftreats 128 like "no match", any error lets the gate pass. Examples: a git built without PCRE (-Punsupported), an over-long argument list, or a filename that git reads as pathspec magic. I confirmed locally that a bad pattern returnsrc=128, and in this construct the gate would then continue. CI on ubuntu has PCRE, so CI is not at risk today. Local runs can still fail open. Fix: capture the status and fail on anything other than 0 or 1, e.g.git grep ... && rc=0 || rc=$?; ((rc == 1)) || { ((rc == 0)) && echo "...English-only..." >&2; exit 1; }. -
One file on
mainalready contains ideographs (apps/voice-runtime/src/live-worker.test.ts:419)
expect(afterCall("...", "..."))uses literal Chinese strings. This is the only tracked file that matches the new pattern. The gate scans whole changed files, not only changed lines. So the next PR that touches this test, even for an unrelated reason, fails the gate. Converting these two literals to\uescapes here would leavemainclean when the rule lands. -
The path note in the moved section misses one root-level path (
docs/desktop-release.md:146)
The note says paths are relative toapps/desktop/exceptscripts/prepare-native-*.mjs,scripts/source-native.mjsandscripts/installs/. The moved text also citespackages/local-installation/src/native-dependencies.ts, which exists only at the repository root. Add it to the exception list.
Verified as fine
- The moved section matches the old
apps/desktop/AGENTS.md"Source Package" and "Release" text line for line. Apart from heading levels and one trailing blank line, the only change is the link todocs/desktop-release.md, which became#how-the-workflow-runs. That anchor exists, and so does#packaging-details, which the desktop pointer and the release doc now link to. - The relative paths
scripts/build-teamhub.mjs,scripts/build-native-runtime.mjs,packaging/...andsrc/main/distribution.test.tsall exist underapps/desktop/.scripts/prepare-native-*.mjs,scripts/source-native.mjsandscripts/installs/stack.shexist at the root, which matches the note. - Root
AGENTS.md: the rewording keeps every rule. "Fast checks" is defined once. The English-only scope (docs, code, comments, logs, strings, prompts, Skills, tests, fixtures, benchmark data) is preserved in a single place, and the removed sentence "Comments are English" is covered by it. - The regex
(*UTF)[\x{3400}-\x{4DBF}\x{4E00}-\x{9FFF}\x{F900}-\x{FAFF}]underLC_ALL=Cmatches ideographs and does not match hiragana or full-width punctuation. I checked this in a temp repo. The published built-in Skill versions skipped by the loop are also excluded from the scan. The check runs beforemake check-scripts-fast check-docs, and the new test asserts both that order and the allowed-kana case. The fixture uses escaped code points, so the test file itself stays English-only. - No user message →
provider_readypath, contracts, env vars or cross-app imports are touched.
Process notes
- The branch contains a merge commit (
01736d9 Merge remote-tracking branch 'origin/main').mainonly takes rebase merges, so please rebase ontomaininstead of merging it in. - The branch is not behind
main.
CI
repository-gate, check (desktop), check (web) and CodeQL were still pending at review time. The other checks were skipped by path filters. I could not run scripts/ci/check-repository-gate.test.ts locally because the worktree has no installed dependencies. Instead I checked the regex and the error exit code directly with git grep in a temp repo.
Automated review by Claude Code
Automated review of 01736d9 by Claude Code.
There was a problem hiding this comment.
Approve (0 findings)
Scope: follow-up on 01736d9, reviewing head abd8340 against main (merge base 1843eff); 4 commits, 6 files in total, with the incremental diff 01736d9..abd8340 (1 commit, 3 files) reviewed in detail. Helper skills loaded: coderbook. ponytail-review and karpathy-guidelines were not loaded for this small follow-up; their checklists were applied from the review instructions. Ran in a separate session.
Previous findings
- P3
scripts/ci/check-repository-gate.sh: the gate passed silently whengit greperrored. Resolved. Lines 68-77 now capture the exit status and handle each case: 0 fails the gate with the English-only message, 1 passes, and any other status fails with "could not run (git grep exited N)". Underset -e, the|| cjk_status=$?form is the right way to keep the status. - P3
apps/voice-runtime/src/live-worker.test.ts:419: a literal Chinese fixture was left on main. Resolved. It now uses\uescapes, andgit grepfinds no ideographs left in the file. - P3
docs/desktop-release.md:146: the relative-path note left out a root-level path. Resolved.packages/local-installation/is now listed as a repository-root path.
Findings
None.
Verified as fine
- The new gate block runs only when
changed_filesis non-empty. That list holds only existing, non-deleted files (--diff-filter=ACMRTplus[[ -f ]]), so pathspecs from deleted files can't cause spurious errors. A very long argument list now fails loudly instead of passing. - The escaped fixture asserts the same strings as before, so the test's behavior is unchanged.
- The existing gate test
rejects changed files with CJK ideographs and allows kanastill covers both the match and no-match paths. There is no test for the error path (status other than 0 or 1). That path is trivial, and this is not worth blocking on. - The branch is up to date with
origin/main(0 commits behind).
Process notes
01736d9 Merge remote-tracking branch 'origin/main'is a merge commit.maintakes rebase merges, which drop merge commits, so followmainby rebasing instead.
CI
All required checks that ran pass: repository-gate, check (desktop), check (web), changes, CodeQL. The other app checks were skipped because their paths are untouched. I did not run any tests locally.
Automated review by Claude Code
Automated review of abd8340 by Claude Code.
The repository is English-only. The repository gate now fails when a changed file contains CJK ideographs. Kana, Hangul, and full-width punctuation stay allowed, so tests can still use unspaced scripts. Co-Authored-By: Claude Code <noreply@anthropic.com>
Claude Code injects apps/desktop/AGENTS.md whenever a desktop file is read, so the Source Package and Release sections move verbatim to the Packaging Details section of docs/desktop-release.md, with a pointer left behind. The root AGENTS.md names one term, fast checks, for what it called cheap gates and fast boundary checks; states the English-only rule once; and describes the shared-configuration and cleanup rules as outcomes. Co-Authored-By: Claude Code <noreply@anthropic.com>
The check treated every non-zero git grep status as "no match", so a git without PCRE let the gate pass. Only status 1 now means clean. The one existing test with literal ideographs uses escaped code points, and the packaging note lists packages/local-installation/ as a root path. Co-Authored-By: Claude Code <noreply@anthropic.com>
abd8340 to
b2df175
Compare
|
Addressed the review in b2df175: the English-only check now fails on any git grep status other than 0 or 1; 🤖 Addressed by Claude Code |
There was a problem hiding this comment.
Approve (0 findings)
Follow-up on abd8340. Scope: base main, merge base 1843eff, 3 commits, 6 files reviewed at b2df175. I loaded coderbook, ponytail-review and karpathy-guidelines as instructions (Skill tool not invoked; the review followed their named sections) and ran the review alone in a separate session.
Previous findings
The previous review had no findings.
What changed since abd8340
git diff abd8340..b2df175 is empty: the final tree is the same as at the previous review. git range-diff shows only a rebase onto the new main (1843eff). In the docs commit, the rebase brought the moved packaging text up to date with main. "When a voice key is saved, Native extracts…" became "On every install and start, whether or not a voice key is saved…", and the paragraph about failed tool preparation and TEAMHUB_VOICE_MEDIA=off was added. Because main itself changed this text, the move picks up the current wording and does not bring back the old wording.
Findings
None.
Verified as fine
- Moved text: I diffed the lines removed from
apps/desktop/AGENTS.mdagainst the lines added todocs/desktop-release.md. Apart from line-wrapping artifacts, the only differences are the ones the PR body lists:- the new
## Packaging Detailsheading and its root-path note; ##→###heading levels;- the release-doc link rewritten to
#how-the-workflow-runs.
- the new
- Pointer:
apps/desktop/AGENTS.md:142now says "see Source Package and Release". It points to the new stub section at line 247, which links to../../docs/desktop-release.md#packaging-details. - Gate script (
scripts/ci/check-repository-gate.sh):- The CJK scan checks only changed, existing files outside published Skill versions, the same set that goes to Prettier.
- Exit status 1 (no match) passes.
- Status 0 (match) and any other status (for example, git without PCRE) fail the gate.
- The ranges cover CJK Ext-A, the Unified block and Compatibility Ideographs. Kana and Hangul stay allowed, as the rule intends.
- Test:
check-repository-gate.test.tscovers rejecting你好(and checks that the later make step never runs) and accepting kana. apps/voice-runtime/src/live-worker.test.ts: the literal Chinese strings became\uescapes. In JS these are the same string values, so the test does the same thing. The file now passes the new gate.
Process notes
No merge commits and no fixup commits. Merge state is CLEAN.
CI
All checks that ran passed: repository-gate, check (desktop), check (web), CodeQL and Analyze. The others were skipped by path filters. No check listed for this PR runs apps/voice-runtime tests, so the escaped-literal change is checked by reading the code only. Because the diff from the previous review is empty, I ran nothing locally.
Automated review by Claude Code
Automated review of b2df175 by Claude Code.
What and why
Claude Code injects a subdirectory
AGENTS.mdwhenever an agent reads a file under that directory, so large nested files cost context on every touch.apps/desktop/AGENTS.md: the Source Package and Release sections move verbatim to a new Packaging Details section ofdocs/desktop-release.md. A pointer stays behind and names when to read it. The file drops from 27 KB to 17 KB. One self-referential link and theapps/desktop/AGENTS.mdlink in the release doc now point to the new section.AGENTS.md:make check.How it was verified
pnpm exec vitest run scripts/ci/check-repository-gate.test.ts, 7 passed. The new case covers both rejected ideographs and allowed kana.make check-docs: all Markdown links resolve. Prettier and commitlint pass.make check-scripts-fasthas one failure,desktop-release.test.ts› "keeps the archive folder's address out of the log". It fails the same way onorigin/mainwithout this change.apps/desktop/build.mjsfollows the pointer and reaches the moved section. A session that works inapps/webdoes not.Boundary
apps/desktop/. A note at the top of the section lists the root-level scripts it mentions.provider_readypath.🤖 Generated with Claude Code