docs: DOC-06 CONTRIBUTING.md onramp (#873)#885
Conversation
Adversarial self-reviewRan through the diff as a hostile reviewer — command correctness, link validity, overlap with AGENTS.md, and Windows gotcha coverage. Cross-checked every command against Findings
Potential nit not acted on
Acted on
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40c3d4f2de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ```bash | ||
| npm run typecheck # vue-tsc type checking | ||
| npm run build # typecheck + vite build | ||
| npx vitest --run --reporter=verbose # unit tests |
There was a problem hiding this comment.
This section says it lists the exact CI commands, but npx vitest --run --reporter=verbose does not match the required CI gate: .github/workflows/reusable-frontend-unit.yml runs npm run test:coverage (with coverage output/threshold enforcement). As written, contributors can get a local green run here and still fail CI on coverage, which defeats the stated purpose of this checklist.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds a root-level contributor onramp (CONTRIBUTING.md) and updates the README to point new contributors to it, consolidating/cleaning up duplicated contribution guidance while keeping AGENTS.md as the authoritative protocol.
Changes:
- Add
CONTRIBUTING.mdwith prerequisites, local setup, test commands, commit conventions, and PR process. - Update
README.mdto link toCONTRIBUTING.mdand remove duplicated contributing bullets.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| README.md | Links to the new contributing onramp and removes duplicate guidance. |
| CONTRIBUTING.md | New contributor guide covering setup, testing, and contribution workflow (with AGENTS.md as source of truth). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ## Running Tests | ||
|
|
||
| All commands assume you are in the repo root unless noted otherwise. | ||
| These are the exact commands CI runs; keep them green locally before pushing. |
There was a problem hiding this comment.
The note "These are the exact commands CI runs" isn’t accurate for the current CI workflows. For example, CI uses npm ci (not npm install) and in the frontend unit workflow it runs npx vite build and npm run test:coverage rather than npm run build / npx vitest --run --reporter=verbose. Suggest either updating this section to match CI exactly, or rewording to say these commands are the recommended local equivalents/minimum gates.
| These are the exact commands CI runs; keep them green locally before pushing. | |
| Treat these as the recommended local equivalents / minimum gates to keep green before pushing. |
|
|
||
| ```bash | ||
| npm run typecheck # vue-tsc type checking | ||
| npm run build # typecheck + vite build |
There was a problem hiding this comment.
The frontend command list is internally inconsistent with the script definitions: npm run build already runs npm run typecheck (per frontend/taskdeck-web/package.json), so listing both commands in sequence implies running typecheck twice. Consider either removing the standalone npm run typecheck from this list, or changing the build line to npx vite build (which matches CI) and keeping npm run typecheck separate.
| npm run build # typecheck + vite build | |
| npx vite build # production build |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new CONTRIBUTING.md file to guide new contributors through local setup, testing, and the PR process, while updating README.md to link to this new guide. Feedback was provided regarding the PowerShell command chaining examples to ensure they do not inadvertently close a user's terminal window upon failure.
| check `$LASTEXITCODE` when you need to stop on failure. Example: | ||
|
|
||
| ```powershell | ||
| npm run typecheck; if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE }; npm run build |
There was a problem hiding this comment.
Using exit $LASTEXITCODE in a command chaining example is problematic for human contributors because it will close their terminal window if the first command fails. While this pattern is used in AGENTS.md for automated environments where exiting the process is desired, for a local setup guide it is better to use a pattern that stops execution without exiting the shell. Since PowerShell 7+ is recommended in the prerequisites, you could also use && which is supported there, unless there is a specific reason to avoid it for human contributors.
| npm run typecheck; if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE }; npm run build | |
| npm run typecheck; if ($?) { npm run build } |
| On Windows PowerShell, chain with `;` and `$LASTEXITCODE` instead of `&&` (see | ||
| the Windows section above). |
There was a problem hiding this comment.
Update this note to reflect the change in the recommended PowerShell chaining pattern to avoid confusion about using $LASTEXITCODE and exit in interactive shells.
| On Windows PowerShell, chain with `;` and `$LASTEXITCODE` instead of `&&` (see | |
| the Windows section above). | |
| On Windows PowerShell, chain with `;` and a success check instead of `&&` (see | |
| the Windows section above). |
…ractive shells
Replace 'exit $LASTEXITCODE' with a non-terminating success check
('if ($LASTEXITCODE -eq 0) { ... }') so the example does not close a
contributor's terminal window on failure. Note the 'exit' pattern as the
automated-script variant, referencing AGENTS.md. Clarify that &&
unsupported on PowerShell 5.1 but works on PowerShell 7+ (we still use
';' for portability). Addresses PR #885 gemini-code-assist feedback on
line 106.
CI workflows use 'npm ci' not 'npm install', and 'npm run test:coverage' with coverage thresholds rather than plain 'vitest --run'. Reword the lead-in to describe the listed commands as recommended local equivalents / minimum gates, point at .github/workflows/ for the real gate, and call out 'npm run test:coverage' as the way to mirror CI precisely. Addresses PR #885 Copilot feedback on line 134.
…ble typecheck
Replace 'npm run build' with 'npx vite build' to mirror the CI step
('npx vite build' in reusable-frontend-unit.yml) and to avoid running
typecheck twice (since 'npm run build' already runs typecheck).
Replace 'npx vitest --run --reporter=verbose' with 'npm run test:coverage'
so the listed command matches CI's coverage-gated run.
Reorder to 'lint -> typecheck -> build -> tests' (CI order), and add a
short note clarifying 'npm run build' is fine but redundant next to
'npm run typecheck'. Update the Windows PowerShell cross-reference to
match the new non-terminating chaining pattern.
Addresses PR #885 Copilot feedback on lines 154/161 and Codex line 155.
Adversarial review + bot-findings triage — round 2Ran a fresh adversarial pass and triaged the three bot reviews Bot findings triage
Fresh adversarial findings (round 2)Pass covered: command correctness vs No new findings beyond the bot set above. Specifically verified:
Commits
CIPushed at HEAD |
Adds a delivery entry for the 8 PROD-00 PRs merged on 2026-04-16 (#884 SEC-28, #885 DOC-06, #887 DOC-07, #886 PERF-09, #888 PERF-10, #889 OPS-29, #890 FE-15, #891 FE-14) with round-2 adversarial review findings: BREACH JWT-in-body correction (compression level Optimal -> Fastest), IPv6/IPv4 healthcheck fix, null-throw sentinel fix, skipRetry opt-out for baseline tests, setpriv entrypoint for upgrade-safe volume ownership. Also bumps the Last Updated date.
…inks - CLAUDE.md Key Docs gains entries for CONFIGURATION_REFERENCE.md (PR #887), CONTRIBUTING.md (PR #885), and SECURITY.md (PR #884) so the post-merge reality is reflected in the agent onboarding surface. - README.md Contributing section cross-links to CONTRIBUTING.md and adds a Security section pointing at SECURITY.md, guarding against a rebase dropping either link.
Adds a delivery entry for the 8 PROD-00 PRs merged on 2026-04-16 (#884 SEC-28, #885 DOC-06, #887 DOC-07, #886 PERF-09, #888 PERF-10, #889 OPS-29, #890 FE-15, #891 FE-14) with round-2 adversarial review findings: BREACH JWT-in-body correction (compression level Optimal -> Fastest), IPv6/IPv4 healthcheck fix, null-throw sentinel fix, skipRetry opt-out for baseline tests, setpriv entrypoint for upgrade-safe volume ownership. Also bumps the Last Updated date.
…inks - CLAUDE.md Key Docs gains entries for CONFIGURATION_REFERENCE.md (PR #887), CONTRIBUTING.md (PR #885), and SECURITY.md (PR #884) so the post-merge reality is reflected in the agent onboarding surface. - README.md Contributing section cross-links to CONTRIBUTING.md and adds a Security section pointing at SECURITY.md, guarding against a rebase dropping either link.
Summary
Adds a CONTRIBUTING.md at the repo root as a friendly onramp for external
contributors. Complements (does not duplicate) AGENTS.md, which remains the
authoritative protocol.
Closes #873.
Sections Covered
scripts/check-git-env.sh, Cygwin-git pitfall, PowerShell&&caveat, staleindex.lockgood first issueandPriority III)Also cross-links CONTRIBUTING.md from README and removes stale duplicate
contributing bullets below the README license separator.
Commits
docs: add CONTRIBUTING.md with prerequisites and setupdocs: add testing, commit, and PR sections to CONTRIBUTINGdocs: cross-link CONTRIBUTING from READMETest plan
ci-required.yml)