-
Notifications
You must be signed in to change notification settings - Fork 4
docs: add contributor pages for the PR review process and AI usage #921
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,114 @@ | ||
| # How this project uses AI | ||
|
|
||
| AI shows up in offlinecv in three places that have nothing to do with each | ||
| other, and conflating them is how people end up with the wrong idea about | ||
| where their résumé goes. They are: **in the product**, **in how the code gets | ||
| written**, and **in the repo's own automation**. | ||
|
|
||
| ## 1. In the product — on-device, opt-in | ||
|
|
||
| The parser itself is not AI. It is a heuristic cascade over pdf.js output | ||
| ([Architecture at a glance](./architecture.md#the-parse-cascade)), and it runs | ||
| with no model at all. | ||
|
|
||
| On top of that sit the optional surfaces that do use a local language model. | ||
| Each is opt-in, and naming them is more useful than counting them, because the | ||
| list grows: flagging where the heuristic parse and the model disagree, | ||
| critiquing résumé quality, rewriting a bullet or a section, recovering a résumé | ||
| the heuristic parser mangled, matching a pasted job description semantically, | ||
| and inferring a posting's sector during a job search. All of them run through | ||
| [WebLLM](https://github.com/mlc-ai/web-llm) on **WebGPU**, inside the tab. If | ||
| the browser has no WebGPU, the surfaces detect that and say so rather than | ||
| falling back to a server. | ||
|
|
||
| One control in this lane deliberately points the other way, and it is the one | ||
| worth knowing about: the rewrite panel offers to hand you its prompt to run in | ||
| a model you already use elsewhere | ||
| ([`export-prompt.ts`](../src/lib/webllm/export-prompt.ts)). What it copies is | ||
| instructions only — no name, no employer, no bullet, asserted against a full | ||
| fixture résumé in `export-prompt.test.ts` — and a clipboard write reaches no | ||
| network. Where you take it afterwards is your own choice rather than our | ||
| default. | ||
|
|
||
| | Model | Size | License | | ||
| |---|---|---| | ||
| | Qwen 2.5 (1.5B) — default | 1630 MB | Apache-2.0 | | ||
| | Gemma 2 (2B) | 1895 MB | Restricted-Community | | ||
| | Llama 3.2 (3B) | 2264 MB | Restricted-Community | | ||
|
|
||
| Registry: [`src/lib/webllm/models.ts`](../src/lib/webllm/models.ts). The | ||
| default is Apache-2.0 deliberately, so a fresh install boots without a licence | ||
| prompt; the other two are gated behind a consent modal that shows the licence | ||
| before anything downloads. | ||
|
|
||
| **What crosses the network is the model, not your résumé.** Choosing a model | ||
| downloads 1.6–2.3 GB of weights from `huggingface.co` and | ||
| `raw.githubusercontent.com` — the one egress this lane has, and the reason the | ||
| size is shown before you commit to it. The prompt, the résumé text, and the | ||
| output stay in the tab. The full list of what does leave, across every lane, is | ||
| [What leaves the tab](./architecture.md#what-leaves-the-tab). | ||
|
|
||
| ## 2. In how the code gets written — Claude Code, in the open | ||
|
|
||
| This repo is developed with [Claude Code](https://claude.ai/code), and the | ||
| configuration is committed rather than kept on someone's laptop: `CLAUDE.md` | ||
| (the house rules the model reads before writing anything), | ||
| [`.claude/settings.json`](../.claude/settings.json), 18 skill files under | ||
| `.claude/skills/`, and the hooks under [`scripts/hooks/`](../scripts/hooks/) | ||
| that fire on edit, on commit, and when a session ends. Read any of them — they | ||
| are the actual instructions, not a sanitised copy. | ||
|
|
||
| You will not find `Co-Authored-By: Claude` or a generated-with badge anywhere | ||
| in the history. That is a **setting**, not a convention someone remembers to | ||
| follow: `attribution` in `.claude/settings.json` blanks the trailer and the | ||
| badge and drops the session URL, so the text is never emitted. The reasoning — | ||
| including why prose telling a model to suppress its own attribution kept | ||
| losing — is in | ||
| [AI attribution](./CONTRIBUTING-PROCESS.md#ai-attribution--suppressed-by-configuration-not-by-prose). | ||
|
|
||
| **You are not required to use it.** Contribute with whatever you like. The | ||
| gates in [How a change gets reviewed and | ||
| merged](./pr-review-process.md) do not know or care which editor produced the | ||
| diff, and they are the same for everyone. | ||
|
|
||
| ## 3. In the repo's automation — less than you would guess | ||
|
|
||
| The repo's own jobs are ordinary code. The comment moderator | ||
| ([`moderate-comments.yml`](../.github/workflows/moderate-comments.yml)) is an | ||
| account-age threshold plus a list of regexes, and it only ever *hides* a | ||
| comment, reversibly, and only from non-members — no model scores anything. The | ||
| claim-releaser ([`stale-claims.yml`](../.github/workflows/stale-claims.yml)) is | ||
| a date comparison. Neither is AI, and both are deliberately the kind of thing | ||
| you can read in one sitting and predict exactly. | ||
|
|
||
| ## What AI does not get to decide | ||
|
|
||
| A PR here may well get an AI review, and that review is **advisory**. Three | ||
| things stop it from being anything more, and two of the three are enforced by | ||
| configuration rather than by anyone remembering: | ||
|
|
||
| - Branch protection requires **one approving review** from a human before a PR | ||
|
s-annam marked this conversation as resolved.
|
||
| can merge. A repo admin can bypass it, so read this one as enforced for | ||
| contributors and a convention for maintainers. | ||
| - GitHub refuses an approval from a PR's own author, so a run that opens a PR | ||
| and reviews it cannot approve it — the review posts as a plain comment | ||
| carrying its real verdict. | ||
| - **The merge is always a person's action.** Nothing in this repo auto-merges. | ||
|
|
||
| So the useful way to read an AI review is as a very thorough first pass that | ||
| has already checked the mechanical things — did the fixture leak contact data, | ||
| is that a raw `<button>`, does the description match the diff — leaving a human | ||
| reviewer the judgement calls. If you disagree with a finding, say so on the | ||
| thread; a finding that does not reproduce gets dropped, and that is a normal | ||
| outcome, not an argument you have to win. | ||
|
|
||
| ## If you contribute with AI | ||
|
|
||
| Use it. Two things are yours regardless of what wrote the diff: | ||
|
|
||
| 1. **You own the change.** "The model wrote it" is not a review response. If | ||
| you cannot explain why a line is there, it is not ready. | ||
| 2. **Never paste a real résumé into anything committed.** Test with your own | ||
| locally as much as you like; the fixture PII policy is absolute and | ||
| `npm run check:fixtures` enforces part of it, but it cannot tell whether a | ||
| *name* is synthetic. That judgement stays yours. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| # How a change gets reviewed and merged | ||
|
|
||
| The path a pull request takes from "I pushed a branch" to "it is in `main`", | ||
| and the two rules along it that surprise people. Companion to | ||
| [Architecture at a glance](./architecture.md) — that page tells you where your | ||
| change goes, this one tells you what happens to it afterwards. | ||
|
|
||
| ## The path | ||
|
|
||
| ``` | ||
| claim an issue comment on it; a maintainer adds `status:claimed` | ||
| ↓ | ||
| branch + commit <your-initials>/<short-slug>, conventional prefix | ||
| ↓ | ||
| open a PR against main title in conventional form, `Resolves #<n>` in the body | ||
| ↓ | ||
| CI runs `verify` the one required check — see below | ||
| ↓ | ||
| review findings sorted Blocking / Secondary / Nit | ||
| ↓ | ||
| one approval required; a later push dismisses it | ||
| ↓ | ||
| merge queue squash, all-green, up to 5 built at once | ||
| ↓ | ||
| main | ||
| ``` | ||
|
|
||
| Claiming is the step people skip. An issue with the `status:claimed` label | ||
| already has someone on it, and a claim with no linked PR and no activity for | ||
| **7 days** is released automatically by | ||
| [`stale-claims.yml`](../.github/workflows/stale-claims.yml). Full rules: | ||
| [Claiming an issue](../CONTRIBUTING.md#claiming-an-issue). | ||
|
|
||
| ## What CI actually checks | ||
|
|
||
| Branch protection on `main` requires exactly **one** status check, the `verify` | ||
| job in [`ci.yml`](../.github/workflows/ci.yml). That single name hides the | ||
| steps below, and knowing which one failed saves you a guess: | ||
|
|
||
| | Step | Fails when | | ||
| |---|---| | ||
| | `npm run typecheck` | TypeScript does not compile | | ||
| | `npm run lint` | ESLint — including the design-system and colour-token rules | | ||
| | `npm run check:fixtures` | a committed résumé PDF carries real contact data | | ||
| | `npm run check:baselines` | a known-failure exemption is charged to a closed issue | | ||
| | `npm run check:core` | the publishable `@offlinecv/core` tarball is broken | | ||
| | `npm run test:coverage` | a test fails | | ||
| | `npm run build` | the production bundle does not build | | ||
| | fallow static analysis | never — the audit's exit code is swallowed, and the report is uploaded to code scanning | | ||
|
|
||
| `check:fixtures` runs **before** the test suite on purpose: the repo is public, | ||
| so a leaked PDF is the one failure that cannot be undone by a follow-up commit. | ||
| See the PII policy in | ||
| [CONTRIBUTING-PROCESS.md](./CONTRIBUTING-PROCESS.md#test-fixtures--pii-policy-non-negotiable) | ||
| before you add any fixture. | ||
|
|
||
| Run the local gates first — `npm run verify:quick` while you iterate, | ||
| `npm run verify` before you push (a `pre-push` hook runs it for you). They are | ||
| not the sequence above: `verify` scopes the test run to the files you changed, | ||
| where CI always runs the whole suite. Treat a green `verify` as a fast | ||
| pre-flight, not a promise that CI will pass. | ||
|
|
||
| ## What a reviewer is looking for | ||
|
|
||
| Findings are sorted into three buckets, and the label is a claim about the | ||
| merge, not about tone: | ||
|
|
||
| - **Blocking** — must change before merge: a bug that fires on normal use, real | ||
| contact data in a fixture, a hardcoded colour where a semantic token belongs, | ||
| a raw `<button>` where the design-system primitive belongs, or a description | ||
| that claims behaviour the code does not have. | ||
| - **Secondary** — a real pattern worth fixing, not a merge blocker. | ||
| - **Nit** — style and polish, explicitly non-blocking. | ||
|
|
||
| Two properties of the review are worth knowing as an author. The **issue is the | ||
| spec** — a PR that says `Resolves #N` while an acceptance criterion of #N is | ||
| unimplemented is Blocking, because merging closes the issue and nothing reopens | ||
| it. And the **PR description is read last**, as a claim to check against the | ||
| diff rather than a guide to what to look at, so a description that overstates | ||
| what changed is itself a finding. | ||
|
|
||
| ## The two rules that surprise people | ||
|
|
||
| **One commit per PR.** `main` merges through a merge queue, and the queue's | ||
| enqueue API carries no commit-message fields — so GitHub *derives* the squash | ||
| message from the repo's settings, which concatenate every commit on the branch | ||
| as bullets. A three-commit PR lands `wip` and `fix lint` in `main`'s history | ||
| permanently. Collapse the branch before it reaches the queue; the full | ||
| derivation is in | ||
| [Squash messages](./CONTRIBUTING-PROCESS.md#squash-messages-one-commit-per-pr). | ||
|
|
||
| **A push dismisses the approval.** `main` has dismiss-stale-reviews-on-push | ||
| enabled, so pushing after someone approves sends the PR back to needing one. | ||
| Address every comment in one pass and push once, then reply on the threads — | ||
| not a push per comment. | ||
|
|
||
| ## How long it takes | ||
|
|
||
| Of the **last 100 merged PRs** (snapshot 2026-08-28): median time open **4.7 | ||
| hours**, **86%** merged within a day, median **8** files changed, and **73** | ||
| carried at least one review. Re-derive it yourself rather than trusting the | ||
| snapshot: | ||
|
|
||
| ```bash | ||
| gh pr list --repo offlinecv/OfflineCV --state merged --limit 100 \ | ||
| --json number,createdAt,mergedAt,changedFiles,reviews | ||
| ``` | ||
|
|
||
| Nobody is watching the queue for you, though. If a PR has been sitting, say so | ||
| on the issue or in chat — silence from a reviewer who is mid-review and silence | ||
| from one who has not opened it look identical from the author's side. |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.