docs: add contributor pages for the PR review process and AI usage - #921
Conversation
Deploying offlinecv with
|
| Latest commit: |
e5556d0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://75c2e2d4.offlinecv.pages.dev |
| Branch Preview URL: | https://sa-session-docs-review-and-a.offlinecv.pages.dev |
s-annam
left a comment
s-annam
left a comment
There was a problem hiding this comment.
Verdict: REQUEST_CHANGES (self-review — posted as COMMENT; GitHub blocks a PR author's own APPROVE/REQUEST_CHANGES)
Reviewed against the tree at eae13a3. The description was read last, at gate 3f, so the findings below were formed from the diff and the repo alone. ## Review focus invited an independent re-derivation of the numbers; I took that literally and re-derived every one from the GitHub API and the working tree.
Almost all of it holds, exactly. Independently confirmed: required_status_checks.contexts == ["verify"], dismiss_stale_reviews == true, required_approving_review_count == 1, queue mergeMethod: SQUASH / mergingStrategy: ALLGREEN / maximumEntriesToBuild: 5, squash_merge_commit_message: COMMIT_MESSAGES, STALE_AFTER_DAYS = 7, all three model sizes and licences against models.ts:75-100, 18 committed SKILL.md files (the 19th entry is an untracked symlink into internal/, so a public cloner does see 18 — correct), the attribution block in .claude/settings.json, the three hook events, moderate-comments.yml as an age gate plus a regex list that only ever minimizes and only for non-OWNER/MEMBER/COLLABORATOR, allow_auto_merge: false with no gh pr merge anywhere under .github/workflows/, and every relative path and cross-doc anchor resolving. The snapshot in How long it takes re-derives to 4.68h median / 86.0% / 8 files / 73 reviewed over #666–#920 — four for four, to the digit.
npm run verify is green here (exit 0; fallow: ✓ No issues in 4 changed files), so the Test plan boxes are honest.
What follows is what did not survive that pass. One blocker, and it is in the paragraph the page exists for.
Blocking
1. docs/ai-usage.md:14 — "three optional surfaces" undercounts the shipped tree by three. The page opens by saying that conflating AI's roles "is how people end up with the wrong idea about where their résumé goes", then enumerates the product surfaces exhaustively. Six model-driven lanes ship today, unflagged. Details inline.
Secondary
2. docs/ai-usage.md:78 — "Branch protection requires one approving review … before a PR can merge" is not mechanically true. enforce_admins is false; admin bypass on a green self-authored PR is this repo's documented practice. The bullets are framed as guarantees nobody has to remember, and this one is a convention.
3. docs/pr-review-process.md:57 — "Run the same gates locally first" is the one line here that can actively mislead. verify is deliberately not the CI sequence (#828), and CLAUDE.md says so in as many words.
4. docs/ai-usage.md ships orphaned — nothing in the repo links to it. Its sibling page got two inbound links in this same diff; this one got none.
5. docs/pr-review-process.md:102-104 — the re-derive command cannot reproduce one of the four figures it is offered for. reviews is missing from the --json list. Suggestion block inline.
6. docs/pr-review-process.md:37 — "nine steps" matches neither the eight-row table below it nor the twelve steps in ci.yml.
Nits
Non-blocking: ai-usage.md:64 ("scheduled jobs" mislabels moderate-comments.yml, which is on: issue_comment), ai-usage.md:33 ("the CDN", singular, vs the two hosts named in the table it links to), architecture.md:122 (the new item 4 turns the pre-existing ordered list loose), pr-review-process.md:49 (fallow's "never" is true of the run step, not the upload step that follows it). All inline.
One more with no line to anchor to — the description says "All 11 relative links … were verified." The diff touches 15 relative links across the two new pages (6 + 9), or 17 counting the two wiring links added to CONTRIBUTING.md and docs/architecture.md. Every one of them resolves — I checked each path and each anchor — so nothing is broken; the count just undersells the work.
Where I disagree with the generic pass. /code-review scored the orphaned page (#4) as Blocking. I have it as Secondary: the page is committed and correct, the fix is two lines in README.md, and this repo scores the equivalent defect in code — a correct export nothing imports — as a fallow Secondary. It does not change the verdict either way.
Nothing was auto-fixed or pushed (≥1 Blocking, so Step 5.5 does not run). The branch is already at one commit (eae13a3), so the one-commit invariant is satisfied and no collapse is needed.
Gates: 3a fixture PII n/a (no binaries — docs/ and CONTRIBUTING.md only), 3b design-system n/a, 3c style tokens n/a, 3d fallow clean, 3e command-level bugs run (finding 5), 3f description accuracy run — the body is accurate about what it built, and it faithfully reproduces two of the errors above (nine steps, three optional surfaces) rather than papering over them. AC checklist: Refs #901 is non-closing and #901's criteria are about applying good first issue labels, so there is no acceptance criterion for this diff to meet.
Reviewed by: Claude Opus 5 (high)
Two pages aimed at the same first-time contributor `docs/architecture.md` is written for, covering the two things that page deliberately stops short of: what happens to a change after it is pushed, and where AI is and is not involved in this repo. `docs/pr-review-process.md` walks claim -> branch -> PR -> `verify` -> review -> approval -> merge queue, breaks the single required check out into a table of the steps behind it so a red job names itself, states the Blocking/Secondary/Nit sort, and spells out the two rules that reliably catch people: one commit per PR (the queue derives the squash message from the commits) and a push dismissing an approving review. It also says what the local gates are not - `npm run verify` scopes its test run to the files you changed, where CI always runs the whole suite, so a green `verify` is a pre-flight and not a promise (#828). Timings are a stated snapshot of the last 100 merged PRs with the command to re-derive all four of them. `docs/ai-usage.md` separates the three places AI appears, which are routinely conflated: the optional on-device WebLLM surfaces in the product, Claude Code in development with its configuration committed and attribution suppressed by setting rather than by prose, and the repo automation - which is regex and date arithmetic, not a model. The product surfaces are named rather than counted, because the list grows: parse disagreement, resume critique, section rewrite, degenerate-parse recovery, semantic JD match and job-search sector inference. The page also names the one control that points off device - the rewrite panel's exportable prompt, which copies instructions and no resume content. It closes on what AI does not decide: branch protection requires a human approval (with the admin bypass named rather than implied), GitHub refuses an approval from a PR's own author, and nothing here auto-merges. Both link to the existing canonical sources rather than restating them. `CONTRIBUTING.md` and the architecture page point at the review page from the two places a contributor is already reading when the question comes up, and `README.md` points at both new pages so neither is reachable only by browsing `docs/`. Refs #901
81fc585 to
e5556d0
Compare
2 participants
Summary
Two contributor pages for the same first-time reader
docs/architecture.md(#920) is written for, covering the two things that page deliberately stops short of. Both were written as source material for the Bapatla Engineering College student session — slide 11 (PR review process) and slide 12 (AI usage) — but they are written as repo docs, not as slide notes: everything in them is derived from what is already public in this repository, and each page links to the canonical source rather than restating it.docs/pr-review-process.md— what happens to a change after it is pushed:verify→ review → one approval → merge queue →main.verify), and the table names each step behind it and what makes it fail, so a red job identifies itself. It also says whycheck:fixturesruns before the test suite — a leaked PDF on a public repo is the one failure a follow-up commit cannot undo.Resolvesis Blocking), and the PR description is read last, as a claim to check against the diff.ghcommand to re-derive them rather than trust the number.docs/ai-usage.md— separates the three places AI appears here, which get conflated:src/lib/webllm/models.ts, why the Apache-2.0 default matters (no consent gate on a fresh install), and the direction of travel: weights come in, résumé text does not go out.CLAUDE.md,.claude/settings.json, 18 skills, thescripts/hooks/hooks), and attribution suppressed by setting rather than by prose. Explicitly not required of contributors.moderate-comments.ymlis an account-age threshold plus regexes and only ever hides, reversibly;stale-claims.ymlis date arithmetic. Neither is a model, and the page says so rather than letting the reader assume otherwise.It closes on what AI does not decide — one human approval is required, GitHub refuses an approval from a PR's own author, and nothing here auto-merges — and on the two things a contributor owns regardless of what wrote the diff.
Wiring:
CONTRIBUTING.md§ Pull requests and the architecture page's Making your first change now point at the review page, which are the two places a contributor is already reading when the question comes up.README.mdpoints at both new pages — the AI page from the contributor-entry paragraph and again from On-device AI (WebGPU) in dev, so neither ships reachable only by browsingdocs/.Review focus
verifystep table against.github/workflows/ci.yml, the model table againstsrc/lib/webllm/models.ts:75-99, and the branch-protection/merge-queue facts (required_status_checks.contexts == ["verify"],dismiss_stale_reviews == true,required_approving_review_count == 1, queueSQUASH/ALLGREEN/maximumEntriesToBuild: 5).docs/ai-usage.md. Two proportions were deliberately removed during drafting ("most of this repo is written with…", "a model reviews most PRs") because neither is checkable from the public repo. Flag anything else that reads as a confident number without a source.Test plan
npm run verify— green via thepre-pushhook: typecheck, lint,check:nul,check:fixtures,check:baselines,check:core, tests, build, fallow (No issues in 4 changed files).npm run verify:quick— 383 test files / 6450 tests passed.docs/architecture.mdandCONTRIBUTING.md.docs/andCONTRIBUTING.mdonly.Refs #901
Revision (review round 1). All ten review threads addressed in one pass. The load-bearing one: the product section counted three model surfaces where six ship (semantic JD match via
useJdMatch→run-llm-match.ts, sector inference atjob-search/sector.ts:311, and degenerate-parse recovery viaLlmEscapeHatchPanel), and it did not nameexport-prompt.ts— the one in-product control that points at an off-device model. Also corrected: the one-approval bullet now names the admin bypass (enforce_admins: false), the local-gates paragraph no longer impliesverifyis the CI sequence (#828),docs/ai-usage.mdis now reachable fromREADME.md, the re-derive command carries thereviewsfield it needs, the wrong step count is gone,moderate-comments.ymlis no longer described as scheduled, the model CDN is named as its two real hosts, and the new list item no longer turns the pre-existing ordered list loose.