diff --git a/ci-review-prompt.md b/ci-review-prompt.md index 31bab2b..f1aa64e 100644 --- a/ci-review-prompt.md +++ b/ci-review-prompt.md @@ -137,6 +137,34 @@ names such as `id`, `name`, `type`, `value`, `data`, `user`, `order`, `group`, or `key` when a two-word snake_case, camelCase, PascalCase, or local-equivalent name would reduce ORM, SQL reserved-word, serialization, or portability risk. +Identifier exposure and enumeration safety is a security blocker, not a style +note. When a primary key or any identifier that appears in an API response, URL +path or query, redirect, filename, cache key, or other client-visible surface +is a sequential or auto-incrementing integer (SERIAL/BIGSERIAL, AUTO_INCREMENT, +IDENTITY, or an ORM auto-increment `id`), return REQUEST_CHANGES: sequential +ids let attackers enumerate and reach other records (IDOR/enumeration — the +Coupang breach exploited guessable sequential ids). Require a non-sequential, +non-guessable identifier at every exposed boundary — a random UUIDv4 or random +token; treat time-ordered ULID/UUIDv7 as acceptable only when creation-order +leakage is harmless. An internal-only auto-increment key is acceptable solely +when it is never exposed and a separate opaque identifier is used at every +external boundary; when exposure is unclear, treat it as exposed. + +Require every newly added or renamed identifier — tables, columns, keys, +indexes, constraints, API fields, event names, config keys, routes, classes, +functions, methods, variables, files, generated models, and serialized +contracts — to be composed of two or more meaningful words, never a bare single +word or reserved word, in the idiomatic case of that file's language: +snake_case for Python/Ruby/Rust/SQL and DB columns, camelCase for +JavaScript/TypeScript/Java/Kotlin/Swift members, PascalCase for types/classes +and Go exported names, SCREAMING_SNAKE_CASE for constants; follow the +repository's existing convention where it differs and never force one language's +casing onto another. A single-word or reserved name such as `id`, `data`, +`user`, `type`, `value`, `run`, `handler`, or `temp` is a blocker when a +two-word equivalent such as `order_item_id`, `projectId`, `UserProfile`, or +`parseRequest` is clearer and safer. Short-lived loop indices and idiomatic +single-letter math variables are exempt. + Use these severity meanings in human-readable findings and in the control block: diff --git a/code-reviewer-prompt.md b/code-reviewer-prompt.md index 25dc481..01c34e5 100644 --- a/code-reviewer-prompt.md +++ b/code-reviewer-prompt.md @@ -147,6 +147,34 @@ names such as `id`, `name`, `type`, `value`, `data`, `user`, `order`, `group`, or `key` when a two-word snake_case, camelCase, PascalCase, or local-equivalent name would reduce ORM, SQL reserved-word, serialization, or portability risk. +Identifier exposure and enumeration safety is a security blocker, not a style +note. When a primary key or any identifier that appears in an API response, URL +path or query, redirect, filename, cache key, or other client-visible surface +is a sequential or auto-incrementing integer (SERIAL/BIGSERIAL, AUTO_INCREMENT, +IDENTITY, or an ORM auto-increment `id`), flag it as a blocker: sequential ids +let attackers enumerate and reach other records (IDOR/enumeration — the Coupang +breach exploited guessable sequential ids). Require a non-sequential, +non-guessable identifier at every exposed boundary — a random UUIDv4 or random +token; treat time-ordered ULID/UUIDv7 as acceptable only when creation-order +leakage is harmless. An internal-only auto-increment key is acceptable solely +when it is never exposed and a separate opaque identifier is used at every +external boundary; when exposure is unclear, treat it as exposed. + +Require every newly added or renamed identifier — tables, columns, keys, +indexes, constraints, API fields, event names, config keys, routes, classes, +functions, methods, variables, files, generated models, and serialized +contracts — to be composed of two or more meaningful words, never a bare single +word or reserved word, in the idiomatic case of that file's language: +snake_case for Python/Ruby/Rust/SQL and DB columns, camelCase for +JavaScript/TypeScript/Java/Kotlin/Swift members, PascalCase for types/classes +and Go exported names, SCREAMING_SNAKE_CASE for constants; follow the +repository's existing convention where it differs and never force one language's +casing onto another. A single-word or reserved name such as `id`, `data`, +`user`, `type`, `value`, `run`, `handler`, or `temp` is a blocker when a +two-word equivalent such as `order_item_id`, `projectId`, `UserProfile`, or +`parseRequest` is clearer and safer. Short-lived loop indices and idiomatic +single-letter math variables are exempt. + Inspect repository-native execution contracts before choosing verification: `pyproject`, `tox`/`nox`, GitHub Actions matrices, `package.json`/engines/ `.nvmrc`, `Cargo.toml`, `go.mod`, Maven/Gradle files, R `DESCRIPTION`, diff --git a/scripts/ci/opencode_review_prompt_template.md b/scripts/ci/opencode_review_prompt_template.md index 2e1c444..4888b89 100644 --- a/scripts/ci/opencode_review_prompt_template.md +++ b/scripts/ci/opencode_review_prompt_template.md @@ -12,6 +12,8 @@ Review by positive evidence, not by absence of known blockers. APPROVE is valid Find bugs. Compare the PR title, body, linked issue context, and actual diff, then inspect the connected code paths, rendering path, tests, docs, generated artifacts, deployment/operation paths, and previous behavior that the changed code now interacts with. Do not review the changed hunk as an isolated island: look for contradictions between the PR intent and repository code, between docs and code, between API/schema names and consumers, between UI rendering and state/data flow, between tests and implementation, and between generated files and their source of truth. If the PR promises files, tests, docs, migrations, generated artifacts, contracts, or behavior that are absent, request changes. Also infer missing files from source evidence: new imports without implementation, new routes without tests/docs, schema changes without migration/rollback, API or CLI behavior without contract tests, generated artifact sources without regenerated outputs, docs claims without code support, config changes without examples, and workflow/tooling changes without self-tests. When a required file is missing, anchor the finding to the closest changed reference, manifest, test, workflow, route, import, docs claim, or generated-artifact contract and explain exactly which file/artifact must be added or updated. Check correctness, edge cases, error paths, API compatibility, auth/authz, tenant isolation, secrets, privacy, data integrity, concurrency, migrations, deployment/rollback, observability, performance, resource use, dependency license and supply-chain risk, IaC/cloud/Docker behavior, package/build/test/lint/security contracts, repository conventions, accessibility, i18n/l10n, developer experience, and user experience. Check naming and reserved-word safety for every changed database object, table, column, primary key, foreign key, index, constraint, API field, event name, configuration key, route, class, function, method, file path, generated model, and serialized contract. Prefer the repository's existing convention, but require names to be specific, non-reserved, and meaningfully composed: avoid bare `id`, `name`, `type`, `value`, `data`, `user`, `order`, `group`, `key`, or SQL/platform reserved words when a two-word snake_case, camelCase, PascalCase, or local equivalent such as `order_item_id`, `projectId`, or `UserProfile` would be clearer and safer. For database primary keys, foreign keys, join tables, migrations, and generated ORM models, compare nearby schema conventions and flag ambiguous single-word identifiers or reserved words that can cause query, ORM, serialization, or cross-database portability bugs. At the start of review, define the UX and DX surfaces for this PR from evidence. UX surfaces may include web UI, CLI behavior, API responses, SDK/library contracts, generated files, docs, logs, error messages, workflow/status-check output, review comments, configuration, operator runbooks, onboarding/setup, and migration paths. DX surfaces may include local setup, scripts, tests, lint/coverage/security commands, CI reliability, error diagnostics, review feedback quality, package/release contracts, observability for maintainers, code readability, extension points, and conventions. If a surface is absent, name the closest affected human or automation interaction instead of writing "not applicable." For breaking changes, use git history and deployment evidence when available to discuss bridge modules, migration paths, rollout/rollback, and lower-version compatibility. +Identifier exposure and enumeration safety is a security blocker, not a style note: when a primary key or any identifier exposed in an API response, URL path or query, redirect, filename, cache key, or other client-visible surface is a sequential or auto-incrementing integer (SERIAL/BIGSERIAL, AUTO_INCREMENT, IDENTITY, or an ORM auto-increment id), return REQUEST_CHANGES because sequential ids let attackers enumerate and reach other records (IDOR/enumeration — the Coupang breach exploited guessable sequential ids); require a non-sequential, non-guessable identifier at every exposed boundary such as a random UUIDv4 or random token, treat time-ordered ULID/UUIDv7 as acceptable only when creation-order leakage is harmless, and accept an internal-only auto-increment key solely when it is never exposed and a separate opaque identifier is used at every external boundary, treating unclear exposure as exposed. Require every newly added or renamed identifier — tables, columns, keys, indexes, constraints, API fields, event names, config keys, routes, classes, functions, methods, variables, files, generated models, and serialized contracts — to be composed of two or more meaningful words rather than a bare single word or reserved word, in the idiomatic case of that file's language (snake_case for Python/Ruby/Rust/SQL and DB columns, camelCase for JavaScript/TypeScript/Java/Kotlin/Swift members, PascalCase for types/classes and Go exported names, SCREAMING_SNAKE_CASE for constants), following the repository's existing convention where it differs and never forcing one language's casing onto another; a single-word or reserved name such as id, data, user, type, value, run, handler, or temp is a blocker when a two-word equivalent such as order_item_id, projectId, UserProfile, or parseRequest is clearer and safer, while short-lived loop indices and idiomatic single-letter math variables are exempt. + For numerical programming, scientific programming, statistical modeling, simulation, optimization, signal processing, ML metrics, estimators, inference code, or formula-heavy implementations, obtain the original paper, specification, vignette, or authoritative reference through web_search/webfetch or official documentation before approving. Verify that formulas, constants, likelihoods, priors, gradients, convergence criteria, random seeds, tolerances, parameter constraints, and numerical stability tricks match the source or are explicitly justified. Require repo-native or scratch PoC evidence that the implementation recovers true parameters on known synthetic data, including skewed or ill-conditioned true-parameter regimes when the method claims robustness; compare against baseline or prior behavior when available. Strengthen the test case set before approving: do not accept a single happy-path test for one function when the scientific claim depends on multiple regimes. Add augmented scratch tests or require repository tests for balanced and skewed parameters, boundary values, degeneracy/zero-variance inputs, random-seed determinism, numerical tolerance, convergence failure, and prior-version or published-example parity as appropriate, then execute the relevant repository test command or sandboxed PoC. Lack of a host toolchain is not a reason to skip execution: provision an isolated Docker, Docker Compose, devcontainer, Nix, or temporary package-install sandbox and run the augmented verification there with no production credentials or persistent repository mutation. If an LLM or patch changes an equation, estimator, loss, distribution, or statistic without source-backed derivation and regression tests that would catch parameter-recovery failure, request changes. For web application surfaces, execute or cite repository-native frontend/backend/E2E evidence when available. Use Playwright when the repository supports it, and review both behavior and rendering: desktop plus one mobile viewport when practical, visual screenshot or toHaveScreenshot evidence, DOM locator assertions using data-testid/role/label selectors, ARIA snapshot or accessibility-tree evidence for changed interactive surfaces when practical, console error/warn collection, failed network request collection, and target-flow interaction proof. If backend and frontend services exist, prefer python3 scripts/ci/sandboxed_web_e2e.py --repo-root "$OPENCODE_SOURCE_WORKDIR" ... and cite SANDBOXED_WEB_E2E_RESULT. For non-web or unsupported surfaces, state the exact missing contract rather than treating partial execution as full evidence.