diff --git a/.claude/hooks/coderabbit-merge-gate.sh b/.claude/hooks/coderabbit-merge-gate.sh new file mode 100755 index 00000000..15727ba4 --- /dev/null +++ b/.claude/hooks/coderabbit-merge-gate.sh @@ -0,0 +1,117 @@ +#!/usr/bin/env bash +# +# coderabbit-merge-gate.sh +# +# PreToolUse(Bash) hook. Gates `gh pr merge` on CodeRabbit having FINISHED its +# review of the PR's head commit ("review-complete" gate): +# +# - CodeRabbit commit status on head == success -> allow (surface findings) +# - status == pending -> BLOCK (still reviewing) +# - status == failure/error -> BLOCK (problem) +# - status absent / cannot verify -> BLOCK (not reviewed yet) +# +# A finished review lets the merge proceed even if it has actionable comments +# (that is the "resolve-all-issues" gate, deliberately not enabled here). The +# count is surfaced so it is not silently ignored. Every failure path fails +# SAFE to BLOCK -- it never silently allows a merge it could not verify. +# +# Blocking uses permissionDecision:"deny" rather than "ask" on purpose: this +# environment runs defaultMode:auto + skipAutoPermissionPrompt, which silently +# auto-approves "ask", making it a no-op. "deny" is a hard block. To override +# (e.g. CodeRabbit is down), edit/remove this hook in .claude/settings.local.json +# or run the merge yourself outside the agent. +# +# Authoritative signal is the `CodeRabbit` commit status (set by commit_status: +# true in .coderabbit.yaml): pending while reviewing, success when complete. A +# new push resets it to pending, so this is inherently staleness-proof. +# +# Reads the hook payload on stdin, emits a PreToolUse decision as JSON on stdout. + +set -o pipefail + +input="$(cat)" +cmd="$(printf '%s' "$input" | jq -r '.tool_input.command // ""' 2>/dev/null)" + +# Fast path: only act when `gh pr merge` is an actual command, not a substring. +# Anchor it to a command boundary (line start, &&, ;, |, then, do) so it still +# catches compound forms (`cd x && gh pr merge 9`) but NOT mentions inside +# `echo "... gh pr merge ..."`, `git commit -m "... gh pr merge ..."`, or +# `rg "gh pr merge"`. Anything else passes untouched. +if ! printf '%s' "$cmd" | grep -qE '(^|&&|;|\||\bthen\b|\bdo\b)[[:space:]]*gh[[:space:]]+pr[[:space:]]+merge([[:space:]]|$)'; then + exit 0 +fi + +# Emit a hard "deny" decision (blocks the merge) and exit. +block() { + jq -nc --arg r "$1" \ + '{hookSpecificOutput:{hookEventName:"PreToolUse",permissionDecision:"deny",permissionDecisionReason:$r}}' + exit 0 +} + +# Inject context for the model but do not block (normal permission flow continues). +note() { + jq -nc --arg c "$1" \ + '{hookSpecificOutput:{hookEventName:"PreToolUse",additionalContext:$c}}' + exit 0 +} + +command -v gh >/dev/null 2>&1 || block "CodeRabbit gate: 'gh' not found — cannot verify CodeRabbit review. Confirm manually before merging." +command -v jq >/dev/null 2>&1 || exit 0 # jq missing: cannot build payload; do not block. + +# Target repo: honor -R/--repo on the merge command, else the current repo. +repo="$(printf '%s' "$cmd" | grep -oE '(-R|--repo)[ =]+[^ ]+' | head -1 | sed -E 's/^(-R|--repo)[ =]+//')" +repo_args=() +[ -n "$repo" ] && repo_args=(--repo "$repo") + +# PR number: prefer pull/ from a URL, then a bare integer argument, else the +# current branch's PR. (A bare-digit token avoids grabbing digits inside an +# owner name such as ".../mohamed-elkholy95/...".) +args="$(printf '%s' "$cmd" | sed -E 's/.*gh[[:space:]]+pr[[:space:]]+merge//')" +pr="$(printf '%s' "$args" | grep -oE 'pull/[0-9]+' | head -1 | grep -oE '[0-9]+')" +if [ -z "$pr" ]; then + pr="$(printf '%s' "$args" | tr ' ' '\n' | grep -xE '[0-9]+' | head -1)" +fi +if [ -z "$pr" ]; then + pr="$(gh pr view "${repo_args[@]}" --json number --jq '.number' 2>/dev/null)" +fi +[ -n "$pr" ] || block "CodeRabbit gate: could not determine the PR for this merge. Confirm CodeRabbit reviewed it, then merge." + +# owner/repo for the commit-status API. +nwo="$repo" +[ -n "$nwo" ] || nwo="$(gh repo view --json nameWithOwner --jq '.nameWithOwner' 2>/dev/null)" +[ -n "$nwo" ] || block "CodeRabbit gate: could not resolve the repository for PR #$pr. Confirm CodeRabbit review, then merge." + +# Head commit of the PR. +sha="$(gh pr view "$pr" "${repo_args[@]}" --json commits --jq '.commits[-1].oid' 2>/dev/null)" +[ -n "$sha" ] || block "CodeRabbit gate: could not read PR #$pr head commit. Confirm CodeRabbit review, then merge." + +# CodeRabbit commit status on the head commit. +cr_state="$(gh api "repos/$nwo/commits/$sha/status" \ + --jq '.statuses[] | select(.context=="CodeRabbit") | .state' 2>/dev/null | head -1)" + +# Latest "Actionable comments posted: N" from CodeRabbit's completion comment. +actionable="$(gh pr view "$pr" "${repo_args[@]}" --json comments --jq ' + [ .comments[] + | select(.author.login=="coderabbitai") + | select(.body | contains("coderabbit-review-completion-marker")) + | (.body | capture("Actionable comments posted: (?[0-9]+)").n) + ] | last // "unknown"' 2>/dev/null)" + +case "$cr_state" in + success) + if [ "$actionable" = "0" ] || [ "$actionable" = "unknown" ] || [ -z "$actionable" ]; then + note "CodeRabbit review complete on PR #$pr (no actionable comments). Proceeding." + else + note "CodeRabbit review complete on PR #$pr with $actionable actionable comment(s). Review-complete gate allows the merge — confirm those were addressed/resolved before merging." + fi + ;; + pending) + block "CodeRabbit is still reviewing the latest commit on PR #$pr (status: pending). Wait for the review to finish before merging." + ;; + failure|error) + block "CodeRabbit commit status on PR #$pr is '$cr_state'. Investigate and resolve before merging." + ;; + *) + block "No CodeRabbit review found on PR #$pr's head commit ($sha). CodeRabbit may not have reviewed this push yet (or is not enabled here). Confirm before merging." + ;; +esac diff --git a/.github/workflows/dispatch-pythinker-home-sync.yml b/.github/workflows/dispatch-pythinker-home-sync.yml index 03f43636..460fd1c2 100644 --- a/.github/workflows/dispatch-pythinker-home-sync.yml +++ b/.github/workflows/dispatch-pythinker-home-sync.yml @@ -1,10 +1,16 @@ name: Dispatch pythinker-home sync # Triggers a pythinker-home website sync when the install scripts or README -# change on main. Release promotion and the post-release sync live in -# promote-release.yml: a GITHUB_TOKEN-created release never fires a workflow, -# so the old `release: published` trigger here was dead code that never ran the -# wait-for-assets / mark-latest steps. +# change on main. This is a best-effort latency optimization, NOT the source of +# truth: pythinker-home re-syncs on its own daily cron (sync-upstream-products +# @ 04:17 UTC) using its own GITHUB_TOKEN, so a failed dispatch here never +# leaves the website stale. The job therefore degrades gracefully (warns + +# alerts, exits 0) instead of red-lining main when the org-owned +# pythinker-release-bot App is missing, uninstalled, or rotated. +# +# Release promotion's post-release sync lives in promote-release.yml: a +# GITHUB_TOKEN-created release never fires a workflow, so the old +# `release: published` trigger here was dead code that never ran. on: workflow_dispatch: @@ -28,99 +34,79 @@ jobs: steps: # Mint a short-lived installation token for the org-owned # pythinker-release-bot App (Contents: write on pythinker-home only). - # Replaces a personal PAT: org-owned (survives member/org changes), - # ~1h TTL, minted fresh each run, scoped to the single private site repo. + # `continue-on-error` is deliberate: a missing/rotated App must not fail + # the run — the next step detects the empty token and degrades gracefully. - name: Mint GitHub App token for pythinker-home id: app-token - env: - APP_ID: ${{ secrets.PYTHINKER_RELEASE_BOT_APP_ID }} - APP_PRIVATE_KEY: ${{ secrets.PYTHINKER_RELEASE_BOT_APP_PRIVATE_KEY }} - DISPATCH_OWNER: ${{ env.DISPATCH_OWNER }} - DISPATCH_REPO: ${{ env.DISPATCH_REPO }} - run: | - set -euo pipefail - if [ -z "${APP_ID:-}" ] || [ -z "${APP_PRIVATE_KEY:-}" ]; then - echo "::error::Missing PYTHINKER_RELEASE_BOT_APP_ID or PYTHINKER_RELEASE_BOT_APP_PRIVATE_KEY. Configure the org-owned pythinker-release-bot App and install it on ${DISPATCH_OWNER}/${DISPATCH_REPO} with Contents: Read and write." >&2 - exit 1 - fi + continue-on-error: true + uses: actions/create-github-app-token@fee1f7d63c2ff003460e3d139729b119787bc349 # pinned from v2.2.2 + with: + app-id: ${{ secrets.PYTHINKER_RELEASE_BOT_APP_ID }} + private-key: ${{ secrets.PYTHINKER_RELEASE_BOT_APP_PRIVATE_KEY }} + owner: ${{ env.DISPATCH_OWNER }} + repositories: ${{ env.DISPATCH_REPO }} + permission-contents: write - b64url() { openssl base64 -A | tr '+/' '-_' | tr -d '='; } - key_file=$(mktemp) - trap 'rm -f "$key_file"' EXIT - printf '%s\n' "$APP_PRIVATE_KEY" > "$key_file" - chmod 600 "$key_file" - - now=$(date +%s) - header=$(printf '{"alg":"RS256","typ":"JWT"}' | b64url) - payload=$(jq -nc --argjson iat "$((now - 60))" --argjson exp "$((now + 540))" --arg iss "$APP_ID" '{iat:$iat,exp:$exp,iss:$iss}' | b64url) - unsigned="${header}.${payload}" - signature=$(printf '%s' "$unsigned" | openssl dgst -sha256 -sign "$key_file" | b64url) - jwt="${unsigned}.${signature}" - - installation_id=$(curl --fail-with-body -sS \ - -H "Accept: application/vnd.github+json" \ - -H "Authorization: Bearer ${jwt}" \ - -H "X-GitHub-Api-Version: 2022-11-28" \ - "https://api.github.com/repos/${DISPATCH_OWNER}/${DISPATCH_REPO}/installation" \ - | jq -er '.id') - token=$(jq -nc --arg repo "$DISPATCH_REPO" '{repositories:[$repo],permissions:{contents:"write"}}' \ - | curl --fail-with-body -sS \ - -X POST \ - -H "Accept: application/vnd.github+json" \ - -H "Authorization: Bearer ${jwt}" \ - -H "X-GitHub-Api-Version: 2022-11-28" \ - "https://api.github.com/app/installations/${installation_id}/access_tokens" \ - -d @- \ - | jq -er '.token') - echo "::add-mask::$token" - echo "token=$token" >> "$GITHUB_OUTPUT" - - - name: Trigger pythinker-home sync + - name: Trigger pythinker-home sync (best-effort) env: DISPATCH_TOKEN: ${{ steps.app-token.outputs.token }} + TOKEN_OUTCOME: ${{ steps.app-token.outcome }} SOURCE_REPO: ${{ github.repository }} RELEASE_TAG: ${{ github.sha }} DISPATCH_OWNER: ${{ env.DISPATCH_OWNER }} DISPATCH_REPO: ${{ env.DISPATCH_REPO }} - run: | - set -euo pipefail - if [ -z "${DISPATCH_TOKEN:-}" ]; then - echo "::error::No dispatch token: the pythinker-release-bot App token mint produced an empty value. Confirm PYTHINKER_RELEASE_BOT_APP_ID and PYTHINKER_RELEASE_BOT_APP_PRIVATE_KEY org secrets are set and the App is installed on ${DISPATCH_OWNER}/${DISPATCH_REPO} with Contents: Read and write." >&2 - exit 1 - fi - payload=$(jq -n \ - --arg source_repo "$SOURCE_REPO" \ - --arg tag "$RELEASE_TAG" \ - '{"event_type":"sync-pythinker-products","client_payload":{"source_repo":$source_repo,"tag":$tag}}') - curl --fail-with-body \ - -X POST \ - -H "Accept: application/vnd.github+json" \ - -H "Authorization: Bearer $DISPATCH_TOKEN" \ - "https://api.github.com/repos/${DISPATCH_OWNER}/${DISPATCH_REPO}/dispatches" \ - -d "$payload" - - notify-failure: - name: Notify on failure - runs-on: ubuntu-latest - needs: dispatch - if: failure() - steps: - - name: Post Slack alert - env: SLACK_WEBHOOK_URL: ${{ secrets.SLACK_WEBHOOK_URL }} RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} - REPO: ${{ github.repository }} - TRIGGER: ${{ github.event_name }} run: | - if [ -z "$SLACK_WEBHOOK_URL" ]; then + # No `set -e`: anticipated failures degrade gracefully (warn + alert + + # exit 0) so a broken App never red-lines main. The daily cron in + # pythinker-home is the real sync guarantee. + set -uo pipefail + + degrade() { + reason="$1" + echo "::warning title=pythinker-home sync skipped::${reason}" + { + echo "### :warning: pythinker-home sync dispatch skipped" + echo "" + echo "${reason}" + echo "" + echo "**Non-blocking:** pythinker-home re-syncs on its daily cron (\`sync-upstream-products\` @ 04:17 UTC) using its own token, so the website is not stale." + echo "" + echo "**Restore the fast path:** recreate/install the **pythinker-release-bot** App on \`${DISPATCH_OWNER}\` with *Contents: write* on \`${DISPATCH_REPO}\`, then update the \`PYTHINKER_RELEASE_BOT_APP_ID\` and \`PYTHINKER_RELEASE_BOT_APP_PRIVATE_KEY\` secrets." + } >> "$GITHUB_STEP_SUMMARY" + if [ -n "${SLACK_WEBHOOK_URL:-}" ]; then + alert=$(jq -n --arg run_url "$RUN_URL" --arg repo "$SOURCE_REPO" --arg reason "$reason" \ + '{"text":":warning: *pythinker-home sync dispatch skipped (non-blocking)*","attachments":[{"color":"warning","fields":[{"title":"Repo","value":$repo,"short":true},{"title":"Reason","value":$reason,"short":false},{"title":"Run","value":"<\($run_url)|View logs>","short":false}]}]}') + curl -sS -X POST -H "Content-Type: application/json" -d "$alert" "$SLACK_WEBHOOK_URL" || true + fi exit 0 + } + + if [ "${TOKEN_OUTCOME}" != "success" ] || [ -z "${DISPATCH_TOKEN:-}" ]; then + degrade "Could not mint a pythinker-release-bot App token (token step outcome: ${TOKEN_OUTCOME}). The App is likely missing/uninstalled on ${DISPATCH_OWNER}, or its credentials are stale." fi - payload=$(jq -n \ - --arg run_url "$RUN_URL" \ - --arg repo "$REPO" \ - --arg trigger "$TRIGGER" \ - '{"text":":red_circle: *Dispatch pythinker-home sync failed*","attachments":[{"color":"danger","fields":[{"title":"Repo","value":$repo,"short":true},{"title":"Trigger","value":$trigger,"short":true},{"title":"Run","value":"<\($run_url)|View logs>","short":false}]}]}') - curl --fail-with-body -X POST \ - -H "Content-Type: application/json" \ - -d "$payload" \ - "$SLACK_WEBHOOK_URL" + + payload=$(jq -n --arg source_repo "$SOURCE_REPO" --arg tag "$RELEASE_TAG" \ + '{"event_type":"sync-pythinker-products","client_payload":{"source_repo":$source_repo,"tag":$tag}}') + + # repository_dispatch returns 204 on success. Retry transient errors; + # a persistent failure degrades (the cron still backstops the sync). + resp=$(mktemp) + code="000" + for attempt in 1 2 3; do + code=$(curl -sS -o "$resp" -w '%{http_code}' \ + -X POST \ + -H "Accept: application/vnd.github+json" \ + -H "Authorization: Bearer $DISPATCH_TOKEN" \ + -H "X-GitHub-Api-Version: 2022-11-28" \ + "https://api.github.com/repos/${DISPATCH_OWNER}/${DISPATCH_REPO}/dispatches" \ + -d "$payload") || code="000" + if [ "$code" = "204" ]; then + echo "Dispatched sync-pythinker-products to ${DISPATCH_OWNER}/${DISPATCH_REPO} (HTTP 204)." + exit 0 + fi + echo "Dispatch attempt ${attempt} returned HTTP ${code}: $(head -c 200 "$resp")" + [ "$attempt" -lt 3 ] && sleep $((attempt * 3)) || true + done + degrade "repository_dispatch to ${DISPATCH_OWNER}/${DISPATCH_REPO} failed after 3 attempts (last HTTP ${code}): $(head -c 200 "$resp")" diff --git a/CHANGELOG.md b/CHANGELOG.md index 278553de..b7e0aa09 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,9 @@ GitHub Releases page; `0.8.0` is the new starting line. ## Unreleased - **Release promotion no longer stalls when the Homebrew tap is broken.** The `promote-release` workflow now gates only on platform assets and PyPI; a lagging or broken Homebrew tap emits a warning annotation and step summary note but no longer blocks the GitHub Release from reaching Latest. +- **Calmer, theme-aligned TUI rendering.** Transcript, recap, and tool-header output now use theme-standardized activity colors instead of hardcoded values, and Markdown tables render as a bordered grid (wide tables no longer collapse into a stacked-record list). +- **Auto-mode tool approval fails closed when unattended.** In auto/non-interactive runs, an action that still needs approval under the active safe-mode/trust policy is denied with guidance instead of waiting indefinitely for an absent user, and outside-workspace writes are never auto-approved. A new `auto_deliberate_destructive_actions` setting can additionally bounce destructive auto-approved actions once for deliberation before they run. +- **`pythinker review` validates finding evidence.** Reviewflow assembles prompts from a shared security-knowledge manifest and validates findings, handling invalid ones without failing the whole review. ## 0.30.0 (2026-06-02) diff --git a/packages/pythinker-review/README.md b/packages/pythinker-review/README.md index 29df516a..dac48d90 100644 --- a/packages/pythinker-review/README.md +++ b/packages/pythinker-review/README.md @@ -25,6 +25,7 @@ pythinker-review show-finding pythinker-review init pythinker-review map pythinker-review review --limit 3 --jobs 3 +pythinker-review review --limit 3 --jobs 3 --prompt-file review-guidance.md --rate-limit-per-minute 6 # optional guidance/rate cap pythinker-review report --status open pythinker-review show --finding pythinker-review triage --finding --status false-positive @@ -100,8 +101,9 @@ The stateful Reviewflow workflow writes `.pythinker-review-flow/` by default: Phase 1 now ports the highest-value behavior from the mounted blackbox repos: -- Reviewflow-style evidence validation rejects findings outside the reviewed chunk/feature, unsafe - paths, stale line ranges, or non-matching evidence snippets. +- Reviewflow-style evidence validation uses line-numbered prompt manifests and rejects findings + outside the reviewed chunk/feature, unsafe paths, omitted/truncated line ranges, or non-matching + evidence snippets; invalid sibling findings are recorded as drops without failing the whole run. - Reviewflow pure-Python stateful commands cover `init`, `map`, `status`, `review`, `ci`, `report`, `show --finding`, `next`, `triage`, `revalidate`, `fix`, `open-pr`, `doctor`, and `clean-locks`. diff --git a/packages/pythinker-review/docs/blackbox-parity.md b/packages/pythinker-review/docs/blackbox-parity.md index 0ed2ff87..79604bce 100644 --- a/packages/pythinker-review/docs/blackbox-parity.md +++ b/packages/pythinker-review/docs/blackbox-parity.md @@ -10,7 +10,7 @@ compatibility. | --- | --- | --- | --- | --- | | `blackbox/clawpatch-main/README.md`, `docs/index.md`, `docs/spec.md` | Review is evidence-first; state is durable; fix/PR flows are explicit follow-ups. | `reviewflow/workflow.py`, `reviewflow/state.py`, `packages/pythinker-review/src/pythinker_review/engine/orchestrator.py`, `store/` | Store round-trip, legacy state migration, stateful init/map/review/report/triage/fix e2e, list/show, fail-closed runner tests. | Diff review still persists `.pythinker-review/`; the stateful Reviewflow workflow uses `.pythinker-review-flow/` by default and non-destructively imports legacy state when needed. | | `blackbox/clawpatch-main/src/prompt.ts` review/fix/revalidate prompts | Bounded context, strict JSON, evidence/reasoning/test-analysis/minimum-fix-scope concepts, plus explicit unified-diff fix plans. | `reviewers/prompts/code_review.system.md`, `reviewers/prompts/debug_review.system.md`, `reviewers/prompts/deslopify_review.system.md`, `reviewers/schema.py`, `store/models.py`, `reviewflow/provider.py` | Reviewer prompt/caller tests, schema round-trip tests, malformed-output retry tests, fix unified-diff e2e. | Stateful feature review uses compact pure-Python prompts rather than a literal TypeScript prompt copy. | -| `blackbox/clawpatch-main/src/review-validation.ts` | Reject stale/out-of-context evidence; never silently persist hallucinated findings. | `reviewers/validation.py`, `engine/runner.py`, `reviewers/schema.py` | Validation tests for escaping paths, out-of-chunk files, out-of-hunk line ranges, and evidence snippets. | Validation is chunk-scoped in Phase 1; full semantic feature-context validation is deferred to whole-repo audit mode. | +| `blackbox/clawpatch-main/src/review-validation.ts` | Reject stale/out-of-context evidence; never silently persist hallucinated findings. Stateful review records schema/evidence drops without failing valid sibling findings. | `reviewers/validation.py`, `engine/runner.py`, `reviewers/schema.py`, `reviewflow/provider.py`, `reviewflow/workflow.py` | Validation tests for escaping paths, out-of-chunk files, out-of-hunk line ranges, evidence snippets, prompt manifests, and non-fatal stateful validation drops. | Diff validation is chunk-scoped; stateful feature review is prompt-manifest-scoped with line-numbered excerpts. Full semantic feature-context validation beyond included excerpts remains deferred. | | `blackbox/clawpatch-main/src/app.ts` | Bounded worker pool, retry malformed model output once, run metadata, partial failure visibility, workflow commands. | `engine/runner.py`, `store/models.py`, `store/findings_store.py`, `reviewflow/workflow.py`, `cli/review.py` | Runner fail-closed/allow-partial tests; store atomicity tests; stateful workflow e2e. | Stateful feature review is intentionally conservative and pure Python; agent enrichment is not yet implemented. | | `blackbox/clawpatch-main/src/types.ts`, `src/mapper.ts`, `src/mappers/task-graph.ts` | Durable project/feature/run/finding/patch records and heuristic feature/task mapping. | `reviewflow/models.py`, `reviewflow/mapping.py`, `reviewflow/provider.py`, `reviewflow/state.py` | Pydantic schema import/type checks, mapper partition/script/state/report unit tests, package lint/typecheck. | Mapper coverage includes source partitioning, nearby-test association, Python console scripts, and broad file-pattern grouping; framework-specific mapper details are compacted rather than byte-identical. | | `blackbox/clawpatch-main/src/selection.ts`, `src/git.ts` | Git-scoped selection (`since`/dirty/range), changed-file focus, path-relative behavior. | `engine/diff_source.py`, `engine/chunker.py`, `reviewflow/workflow.py`, `reviewflow/utils.py` | Git fixture tests for base/staged/working-tree/range and glob filters; stateful changed-file selectors are covered through workflow tests. | Diff review remains hunk-scoped; stateful review is feature-scoped. | @@ -41,6 +41,6 @@ compatibility. | `packages/processor/src/agents/shared.ts` JSON parsing | Malformed/non-array model output is a batch error, not "no findings". | `reviewers/security_review.py`, `engine/runner.py` | Security reviewer retries once then records `malformed_output`; fail-closed runner tests. | Pythinker schema is `{"findings": [...]}` instead of the source scanner's array payload. | | Security prompt core | Static-analysis mindset, trace inputs/imports/mitigations, report only validated exploitable issues. | `reviewers/prompts/security_review.system.md` | Prompt/caller tests and signal scanner tests. | Severity taxonomy maps to Pythinker `critical/high/medium/low/info`. | | Scanner rule metadata/matchers | Deterministic signals are prompt anchors, not findings, and carry rule metadata/reasons/confidence/CWE/severity hints. | `signals/models.py`, `signals/scanner.py` | Secret, shell/RCE, SQL, NoSQL, deserialization, SSRF, path traversal, XSS, redirect, JWT, CORS, debug, prompt-injection, weak-crypto rule tests. | Curated in-process rules replace the source plugin marketplace for Phase 1. | -| Prompt assembly with tech highlights/slug notes/project context | Batch-scoped anchors avoid prompt bloat while preserving signal context. | `signals/tech.py`, `signals/advisor.py`, `reviewers/security_review.py`, `engine/orchestrator.py` | Advisor context, tech detection, and security reviewer prompt tests. | INFO.md/config prompt append remain deferred; built-in tech highlights and slug notes are implemented. | +| Prompt assembly with tech highlights/slug notes/project context | Batch-scoped anchors avoid prompt bloat while preserving signal context. | `security_scan/knowledge.py`, `security_scan/tech.py`, `signals/advisor.py`, `reviewers/security_review.py`, `engine/orchestrator.py` | Advisor context, tech detection, and security reviewer prompt tests. | INFO.md/config prompt append remain deferred for diff review; shared built-in tech highlights and slug notes are implemented. | | Revalidation/verdict workflow | Findings should be validated before being treated as final. | `reviewers/validation.py`, `engine/runner.py`; future read-only `revalidate.py` | Validation and fail-closed runner tests. | Separate saved-finding revalidation is deferred; initial model output is evidence-validated now. | | Export/report/metrics | Machine-readable output and CI gating only on net findings. | `output/json.py`, `output/sarif.py`, `cli/_shared.py` | JSON/SARIF schema tests and threshold exit-code tests. | Markdown PR comments and metrics dashboards are deferred. | diff --git a/packages/pythinker-review/src/pythinker_review/cli/review.py b/packages/pythinker-review/src/pythinker_review/cli/review.py index a4bc1b72..ce6e905f 100644 --- a/packages/pythinker-review/src/pythinker_review/cli/review.py +++ b/packages/pythinker-review/src/pythinker_review/cli/review.py @@ -127,6 +127,17 @@ def _resolve_llm() -> ReviewLLM: raise typer.Exit(code=3) +def _read_text_option(path: Path | None) -> str | None: + if path is None: + return None + try: + return path.read_text(encoding="utf-8") + except OSError as exc: + raise ReviewflowWorkflowError( + f"failed to read --prompt-file {path}: {exc}", "invalid-usage" + ) from exc + + def _emit(fmt: OutputFormat, *, meta: RunMeta, findings: list[Finding], no_color: bool) -> str: if fmt is OutputFormat.json: return render_json(meta, findings) @@ -412,6 +423,20 @@ def review_stateful( dry_run: bool = typer.Option(False, "--dry-run"), include_dirty: bool = typer.Option(False, "--include-dirty"), timeout_s: float = typer.Option(180.0, "--timeout-s", min=1.0), + prompt_file: Path | None = typer.Option( + None, + "--prompt-file", + exists=True, + dir_okay=False, + readable=True, + help="Additional reviewer guidance for stateful feature review.", + ), + rate_limit_per_minute: int | None = typer.Option( + None, + "--rate-limit-per-minute", + min=1, + help="Maximum provider review starts per rolling minute.", + ), repo: Path = typer.Option(Path.cwd(), "--repo", "--root"), state_dir: str = typer.Option(".pythinker-review-flow", "--state-dir"), config: Path | None = typer.Option(None, "--config"), @@ -434,6 +459,8 @@ def review_stateful( mode=mode.value, dry_run=dry_run, per_feature_timeout_s=timeout_s, + custom_prompt=_read_text_option(prompt_file), + rate_limit_per_minute=rate_limit_per_minute, ) ) except ReviewflowWorkflowError as exc: diff --git a/packages/pythinker-review/src/pythinker_review/reviewflow/provider.py b/packages/pythinker-review/src/pythinker_review/reviewflow/provider.py index 3a6b141e..012f8907 100644 --- a/packages/pythinker-review/src/pythinker_review/reviewflow/provider.py +++ b/packages/pythinker-review/src/pythinker_review/reviewflow/provider.py @@ -3,12 +3,17 @@ from __future__ import annotations import json +from dataclasses import dataclass from pathlib import Path +from typing import Any, Literal + +from pydantic import BaseModel, Field, ValidationError from pythinker_review.llm.protocol import ReviewLLM from pythinker_review.reviewers.common import complete_typed_json from pythinker_review.reviewflow.models import ( FeatureRecord, + FeatureReviewFinding, FeatureReviewOutput, FindingRecord, FixPlanOutput, @@ -17,6 +22,8 @@ ) from pythinker_review.reviewflow.utils import read_text_bounded +REVIEW_PROMPT_FILE_CHAR_LIMIT = 24_000 + REVIEW_SYSTEM = ( "You are Pythinker Review running a pure-Python Reviewflow review.\n" "Return strict JSON only. Review is read-only. Report only concrete, actionable findings " @@ -34,29 +41,135 @@ Do not include markdown fences. Keep the diff minimal and scoped to the finding. """ +ReviewPromptFileRole = Literal["owned", "context", "test"] +ReviewDropLayer = Literal["schema", "validation"] + + +@dataclass(frozen=True, slots=True) +class ReviewPromptLineRange: + start_line: int + end_line: int + + +@dataclass(frozen=True, slots=True) +class ReviewPromptFileManifest: + path: str + role: ReviewPromptFileRole + reason: str + bytes: int + included_bytes: int + included_line_ranges: tuple[ReviewPromptLineRange, ...] + truncated: bool + readable: bool + skipped_reason: str | None + included_text: str = "" + + +@dataclass(frozen=True, slots=True) +class ReviewPromptManifest: + max_owned_files: int + max_context_files: int + included_files: tuple[ReviewPromptFileManifest, ...] + omitted_files: tuple[dict[str, str], ...] + prompt_bytes: int + approximate_tokens: int + + +@dataclass(frozen=True, slots=True) +class ReviewPromptBundle: + prompt: str + manifest: ReviewPromptManifest + + +@dataclass(frozen=True, slots=True) +class ReviewDrop: + path: tuple[str | int, ...] + message: str + layer: ReviewDropLayer + + +@dataclass(frozen=True, slots=True) +class PartitionedFeatureReviewResult: + output: FeatureReviewOutput + manifest: ReviewPromptManifest + dropped_findings: tuple[ReviewDrop, ...] + + +class _LooseFeatureReviewOutput(BaseModel): + findings: list[Any] = Field(default_factory=list) + def feature_review_user_prompt( - *, root: Path, feature: FeatureRecord, config: ReviewflowConfig, mode: str + *, + root: Path, + feature: FeatureRecord, + config: ReviewflowConfig, + mode: str, + custom_prompt: str | None = None, ) -> str: - files = ( - feature.owned_files[: config.review.max_owned_files] - + feature.context_files[: config.review.max_context_files] - ) + return build_feature_review_prompt_bundle( + root=root, + feature=feature, + config=config, + mode=mode, + custom_prompt=custom_prompt, + ).prompt + + +def build_feature_review_prompt_bundle( + *, + root: Path, + feature: FeatureRecord, + config: ReviewflowConfig, + mode: str, + custom_prompt: str | None = None, +) -> ReviewPromptBundle: + prompt_files = _collect_prompt_files(feature, config) + included_files: list[ReviewPromptFileManifest] = [] file_blocks: list[str] = [] - for ref in files: - path = root / ref.path - file_blocks.append( - f"## {ref.path}\nReason: {ref.reason}\n```\n{read_text_bounded(path)}\n```" - ) + for path, role, reason in prompt_files: + prompt_file = _prompt_file(root=root, path=path, role=role, reason=reason) + included_files.append(prompt_file.manifest) + file_blocks.append(prompt_file.block) + omitted_files = _omitted_prompt_files(feature, config, {path for path, _, _ in prompt_files}) + valid_evidence_paths = [file.path for file in included_files if file.readable] + custom_block = _custom_prompt_block(custom_prompt) + prompt_context = _manifest_prompt_context( + max_owned_files=config.review.max_owned_files, + max_context_files=config.review.max_context_files, + included_files=included_files, + omitted_files=omitted_files, + ) tests = "\n".join(f"- {test.path} ({test.command or 'no command'})" for test in feature.tests) - return f""" + prompt = f""" Review mode: {mode} Feature JSON: {feature.model_dump_json(by_alias=True, indent=2)} -Relevant tests: +{custom_block}Relevant tests: {tests or "- none detected"} +Review guidance: +- Inspect owned files, context files, and linked tests as one feature slice. +- Treat tests as evidence of intended behavior. If tests contradict a suspected bug, skip it or + downgrade confidence and explain the uncertainty. +- Avoid speculative low-evidence findings. Prefer an empty findings array over a weak guess. +- Deduplicate sibling/root-cause issues: report one finding with multiple evidence refs. +- Evidence paths must be exactly one of the valid paths below. +- When citing line ranges, use the gutter numbers in the Files section. +- Do not cite files or line ranges outside the shown excerpts. If an excerpt is truncated, only cite + lines that appear in the Files section. +- Provide whyTestsDoNotAlreadyCoverThis, suggestedRegressionTest, and minimumFixScope when useful. +- For shell/YAML/subprocess/Markdown command recipes, treat parsed command output as process-exec + code; flag mixed command-capture/fallback output that can concatenate machine-readable values. +{_review_mode_guidance(mode)} + +Valid evidence paths: +{chr(10).join(f"- {path}" for path in valid_evidence_paths) or "- none"} + +Prompt context: +{json.dumps(prompt_context, indent=2)} + Files: {chr(10).join(file_blocks) or "No readable files."} @@ -73,9 +186,8 @@ def feature_review_user_prompt( "startLine": 1, "endLine": 1, "symbol": null, - "quote": "exact snippet" + "quote": "exact snippet or null" }}], - "reasoning": "why this is a real issue", "reproduction": "optional concrete trigger or null", "recommendation": "minimum safe fix", @@ -86,6 +198,186 @@ def feature_review_user_prompt( ] }} """.strip() + prompt_bytes = len(prompt.encode("utf-8")) + manifest = ReviewPromptManifest( + max_owned_files=config.review.max_owned_files, + max_context_files=config.review.max_context_files, + included_files=tuple(included_files), + omitted_files=tuple(omitted_files), + prompt_bytes=prompt_bytes, + approximate_tokens=max(1, prompt_bytes // 4), + ) + return ReviewPromptBundle(prompt=prompt, manifest=manifest) + + +@dataclass(frozen=True, slots=True) +class _PromptFile: + block: str + manifest: ReviewPromptFileManifest + + +def _collect_prompt_files( + feature: FeatureRecord, config: ReviewflowConfig +) -> list[tuple[str, ReviewPromptFileRole, str]]: + output: list[tuple[str, ReviewPromptFileRole, str]] = [] + seen: set[str] = set() + + def add(path: str, role: ReviewPromptFileRole, reason: str) -> None: + normalized = _normalize_prompt_path(path) + if normalized in seen: + return + seen.add(normalized) + output.append((normalized, role, reason)) + + for ref in feature.owned_files[: config.review.max_owned_files]: + add(ref.path, "owned", ref.reason) + for ref in feature.context_files[: config.review.max_context_files]: + add(ref.path, "context", ref.reason) + for test in feature.tests[: config.review.max_context_files]: + add(test.path, "test", test.command or "linked test") + return output + + +def _omitted_prompt_files( + feature: FeatureRecord, config: ReviewflowConfig, included: set[str] +) -> list[dict[str, str]]: + omitted: list[dict[str, str]] = [] + + def add_omitted(path: str, role: str, reason: str) -> None: + normalized = _normalize_prompt_path(path) + if normalized not in included: + omitted.append({"path": normalized, "role": role, "reason": reason}) + + for ref in feature.owned_files[config.review.max_owned_files :]: + add_omitted(ref.path, "owned", "maxOwnedFiles") + for ref in feature.context_files[config.review.max_context_files :]: + add_omitted(ref.path, "context", "maxContextFiles") + for test in feature.tests[config.review.max_context_files :]: + add_omitted(test.path, "test", "maxContextFiles") + return omitted + + +def _prompt_file(*, root: Path, path: str, role: ReviewPromptFileRole, reason: str) -> _PromptFile: + full_path = _safe_prompt_path(root, path) + if full_path is None: + manifest = ReviewPromptFileManifest( + path=path, + role=role, + reason=reason, + bytes=0, + included_bytes=0, + included_line_ranges=(), + truncated=False, + readable=False, + skipped_reason="unsafe path", + ) + return _PromptFile( + block=f"## {path}\nRole: {role}\nReason: {reason}\n[unsafe path]", + manifest=manifest, + ) + try: + text = full_path.read_text(encoding="utf-8", errors="replace") + except OSError as exc: + manifest = ReviewPromptFileManifest( + path=path, + role=role, + reason=reason, + bytes=0, + included_bytes=0, + included_line_ranges=(), + truncated=False, + readable=False, + skipped_reason=f"unreadable: {exc.__class__.__name__}", + ) + return _PromptFile( + block=f"## {path}\nRole: {role}\nReason: {reason}\n[unreadable]", manifest=manifest + ) + included = text[:REVIEW_PROMPT_FILE_CHAR_LIMIT] + truncated = len(included) < len(text) + numbered = _line_numbered(included) + line_count = max(1, included.count("\n") + (0 if included.endswith("\n") else 1)) + manifest = ReviewPromptFileManifest( + path=path, + role=role, + reason=reason, + bytes=len(text.encode("utf-8")), + included_bytes=len(included.encode("utf-8")), + included_line_ranges=(ReviewPromptLineRange(start_line=1, end_line=line_count),), + truncated=truncated, + readable=True, + skipped_reason=None, + included_text=included, + ) + trailer = "\n[truncated: only the lines above are valid evidence]" if truncated else "" + block = f"## {path}\nRole: {role}\nReason: {reason}\n```text\n{numbered}{trailer}\n```" + return _PromptFile(block=block, manifest=manifest) + + +def _safe_prompt_path(root: Path, path: str) -> Path | None: + try: + root_resolved = root.resolve() + full_path = (root / path).resolve() + full_path.relative_to(root_resolved) + except (OSError, ValueError): + return None + return full_path + + +def _line_numbered(text: str) -> str: + lines = text.splitlines() + if not lines: + lines = [""] + return "\n".join(f"{idx:>5} | {line}" for idx, line in enumerate(lines, start=1)) + + +def _manifest_prompt_context( + *, + max_owned_files: int, + max_context_files: int, + included_files: list[ReviewPromptFileManifest], + omitted_files: list[dict[str, str]], +) -> dict[str, object]: + return { + "maxOwnedFiles": max_owned_files, + "maxContextFiles": max_context_files, + "includedFiles": [ + { + "path": file.path, + "role": file.role, + "reason": file.reason, + "bytes": file.bytes, + "includedBytes": file.included_bytes, + "includedLineRanges": [ + {"startLine": item.start_line, "endLine": item.end_line} + for item in file.included_line_ranges + ], + "truncated": file.truncated, + "readable": file.readable, + "skippedReason": file.skipped_reason, + } + for file in included_files + ], + "omittedFiles": omitted_files, + } + + +def _custom_prompt_block(custom_prompt: str | None) -> str: + if custom_prompt is None or not custom_prompt.strip(): + return "" + return f"Additional reviewer guidance from --prompt-file:\n{custom_prompt.strip()}\n\n" + + +def _review_mode_guidance(mode: str) -> str: + if mode != "deslopify": + return "" + return """- Deslopify mode: report only concrete simplification findings in category + maintainability or performance. +- Do not look for general bugs, security issues, API contract problems, or hypothetical edge cases. +- Findings must remove real complexity or measurable waste without changing behavior.""" + + +def _normalize_prompt_path(path: str) -> str: + return path.replace("\\", "/").removeprefix("./").rstrip("/") async def review_feature( @@ -96,17 +388,81 @@ async def review_feature( config: ReviewflowConfig, mode: str, timeout_s: float, + custom_prompt: str | None = None, ) -> FeatureReviewOutput: + return ( + await review_feature_partitioned( + llm=llm, + root=root, + feature=feature, + config=config, + mode=mode, + timeout_s=timeout_s, + custom_prompt=custom_prompt, + ) + ).output + + +async def review_feature_partitioned( + *, + llm: ReviewLLM, + root: Path, + feature: FeatureRecord, + config: ReviewflowConfig, + mode: str, + timeout_s: float, + custom_prompt: str | None = None, +) -> PartitionedFeatureReviewResult: + bundle = build_feature_review_prompt_bundle( + root=root, + feature=feature, + config=config, + mode=mode, + custom_prompt=custom_prompt, + ) result = await complete_typed_json( llm=llm, system=REVIEW_SYSTEM, - user=feature_review_user_prompt(root=root, feature=feature, config=config, mode=mode), + user=bundle.prompt, timeout_s=timeout_s, - output_type=FeatureReviewOutput, + output_type=_LooseFeatureReviewOutput, ) if not result.ok or result.output is None: raise RuntimeError(result.failure_message or result.failure_reason or "review failed") - return result.output + output, drops = _partition_review_output(result.output) + return PartitionedFeatureReviewResult( + output=output, + manifest=bundle.manifest, + dropped_findings=drops, + ) + + +def _partition_review_output( + output: _LooseFeatureReviewOutput, +) -> tuple[FeatureReviewOutput, tuple[ReviewDrop, ...]]: + findings: list[FeatureReviewFinding] = [] + drops: list[ReviewDrop] = [] + for idx, candidate in enumerate(output.findings): + try: + findings.append(FeatureReviewFinding.model_validate(candidate)) + except ValidationError as exc: + drops.append( + ReviewDrop( + path=("findings", idx), + message=_format_validation_error(exc), + layer="schema", + ) + ) + return FeatureReviewOutput(findings=findings), tuple(drops) + + +def _format_validation_error(error: ValidationError) -> str: + first = error.errors()[0] if error.errors() else None + if first is None: + return "schema validation failed" + loc = ".".join(str(item) for item in first.get("loc", ())) or "" + msg = str(first.get("msg", "schema validation failed")) + return f"{loc}: {msg}" async def revalidate_finding( @@ -193,8 +549,17 @@ def validation_commands_for_feature(feature: FeatureRecord, config: ReviewflowCo __all__ = [ + "PartitionedFeatureReviewResult", + "REVIEW_PROMPT_FILE_CHAR_LIMIT", + "ReviewDrop", + "ReviewPromptFileManifest", + "ReviewPromptManifest", + "ReviewPromptBundle", + "build_feature_review_prompt_bundle", + "feature_review_user_prompt", "plan_fix", "review_feature", + "review_feature_partitioned", "revalidate_finding", "validation_commands_for_feature", ] diff --git a/packages/pythinker-review/src/pythinker_review/reviewflow/workflow.py b/packages/pythinker-review/src/pythinker_review/reviewflow/workflow.py index 63ec21f2..6fa03869 100644 --- a/packages/pythinker-review/src/pythinker_review/reviewflow/workflow.py +++ b/packages/pythinker-review/src/pythinker_review/reviewflow/workflow.py @@ -34,9 +34,11 @@ derive_finding_triage, ) from pythinker_review.reviewflow.provider import ( + ReviewDrop, + ReviewPromptManifest, plan_fix, revalidate_finding, - review_feature, + review_feature_partitioned, validation_commands_for_feature, ) from pythinker_review.reviewflow.reporting import ( @@ -75,7 +77,6 @@ discover_git, git_output, now_iso, - read_text_bounded, run_id, run_process, run_shell_command, @@ -99,6 +100,27 @@ def __init__(self, message: str, code: str = "workflow-error") -> None: self.code = code +class _AsyncRateLimiter: + """Small in-process start-rate limiter for provider calls.""" + + def __init__(self, per_minute: int | None) -> None: + self._interval_s = 0.0 if per_minute is None or per_minute <= 0 else 60.0 / per_minute + self._lock = asyncio.Lock() + self._next_start_at = 0.0 + + async def wait(self) -> None: + if self._interval_s <= 0: + return + async with self._lock: + loop = asyncio.get_running_loop() + now = loop.time() + if self._next_start_at > now: + await asyncio.sleep(self._next_start_at - now) + now = loop.time() + self._next_start_at = max(self._next_start_at, now) + self._interval_s + + +_NONFATAL_REVIEW_ERROR_CODES = {"schema-drop", "validation-drop"} _DEFAULT_STATE_DIR = ".pythinker-review-flow" _LEGACY_STATE_DIR = ".clawpatch" @@ -323,6 +345,8 @@ async def review_project( mode: str = "default", dry_run: bool = False, per_feature_timeout_s: float = 180.0, + custom_prompt: str | None = None, + rate_limit_per_minute: int | None = None, ) -> dict[str, Any]: loaded = load_project_state(root=root.resolve(), state_dir=state_dir, config_path=config_path) features = select_review_features( @@ -346,6 +370,7 @@ async def review_project( run.claimed_feature_ids = [feature.feature_id for feature in features] write_run(loaded.paths, run) semaphore = asyncio.Semaphore(max(1, min(jobs, max(len(features), 1)))) + limiter = _AsyncRateLimiter(rate_limit_per_minute) finding_ids: list[str] = [] errors: list[RunError] = [] @@ -364,19 +389,33 @@ async def worker(feature: FeatureRecord) -> None: ), allow_non_pending=feature_id is not None, ) - produced = await review_feature( + await limiter.wait() + produced = await review_feature_partitioned( llm=llm, root=loaded.root, feature=locked, config=loaded.config, mode=mode, timeout_s=per_feature_timeout_s, + custom_prompt=custom_prompt, + ) + valid, validation_drops = _validated_review_findings( + loaded.root, + produced.manifest, + produced.output.findings, ) - valid = _validated_review_findings(loaded.root, locked, produced.findings) + for drop in (*produced.dropped_findings, *validation_drops): + errors.append(_drop_run_error(locked.feature_id, drop)) ids = _merge_review_findings(loaded, locked, valid, current_run_id) finding_ids.extend(ids) _mark_feature_reviewed( - loaded, locked, ids, current_run_id, provider=llm.model_display_name + loaded, + locked, + ids, + current_run_id, + provider=llm.model_display_name, + manifest=produced.manifest, + dropped=len(produced.dropped_findings) + len(validation_drops), ) release_feature_lock(loaded.paths, locked.feature_id) locked = None @@ -394,7 +433,8 @@ async def worker(feature: FeatureRecord) -> None: write_feature(loaded.paths, feature) await asyncio.gather(*(worker(feature) for feature in features)) - run.status = "failed" if errors else "completed" + fatal_errors = [error for error in errors if error.code not in _NONFATAL_REVIEW_ERROR_CODES] + run.status = "failed" if fatal_errors else "completed" run.finished_at = now_iso() run.finding_ids = finding_ids run.errors = errors @@ -402,8 +442,10 @@ async def worker(feature: FeatureRecord) -> None: report_path = _write_markdown_report( loaded.paths, read_findings(loaded.paths), read_features(loaded.paths) ) - if errors: - raise ReviewflowWorkflowError(errors[0].message, errors[0].code or "review-failed") + if fatal_errors: + raise ReviewflowWorkflowError( + fatal_errors[0].message, fatal_errors[0].code or "review-failed" + ) return { "run": current_run_id, "reviewed": len(features), @@ -937,40 +979,113 @@ def _new_run(command: str, loaded: LoadedState, current_run_id: str) -> RunRecor ) +def _drop_run_error(feature_id: str, drop: ReviewDrop) -> RunError: + return RunError( + message=( + f"dropped 1 finding from feature {feature_id} at " + f"{'.'.join(str(item) for item in drop.path)}: {drop.message}" + ), + code=f"{drop.layer}-drop", + ) + + def _validated_review_findings( - root: Path, feature: FeatureRecord, findings: list[Any] -) -> list[Any]: - allowed = {ref.path for ref in feature.owned_files} | { - ref.path for ref in feature.context_files - } + root: Path, + manifest: ReviewPromptManifest, + findings: list[Any], +) -> tuple[list[Any], list[ReviewDrop]]: out: list[Any] = [] - for finding in findings: + drops: list[ReviewDrop] = [] + for idx, finding in enumerate(findings): if not finding.evidence: + drops.append( + ReviewDrop( + path=("findings", idx, "evidence"), + message="finding has no evidence", + layer="validation", + ) + ) continue - if all(_valid_evidence(root, evidence, allowed) for evidence in finding.evidence): - out.append(finding) - return out - - -def _valid_evidence(root: Path, evidence: EvidenceRef, allowed: set[str]) -> bool: - if evidence.path not in allowed: - return False + failures = [ + reason + for evidence in finding.evidence + if (reason := _evidence_validation_failure(root, evidence, manifest)) is not None + ] + if failures: + drops.append( + ReviewDrop( + path=("findings", idx, "evidence"), + message=failures[0], + layer="validation", + ) + ) + continue + out.append(finding) + return out, drops + + +def _evidence_validation_failure( + root: Path, evidence: EvidenceRef, manifest: ReviewPromptManifest +) -> str | None: + prompt_file = next( + ( + file + for file in manifest.included_files + if file.path == _normalize_repo_path(evidence.path) + ), + None, + ) + if prompt_file is None: + return f"evidence file was not included in review context: {evidence.path}" + if not prompt_file.readable: + return f"evidence file was not readable in review context: {evidence.path}" try: resolved = (root / evidence.path).resolve() resolved.relative_to(root.resolve()) except ValueError: - return False + return f"evidence file escapes repository root: {evidence.path}" if not resolved.is_file(): - return False - text = read_text_bounded(resolved, limit_chars=100_000) - lines = text.splitlines() - if ( - evidence.start_line is not None - and evidence.end_line is not None - and (evidence.start_line < 1 or evidence.end_line > len(lines)) - ): - return False - return not (evidence.quote and evidence.quote not in text) + return f"evidence file is not readable inside repository: {evidence.path}" + text = resolved.read_text(encoding="utf-8", errors="replace") + if evidence.start_line is None and evidence.end_line is None: + if not evidence.quote or not evidence.quote.strip(): + return f"evidence must include a line range or quote: {evidence.path}" + elif evidence.start_line is None or evidence.end_line is None: + return f"evidence line range must include both startLine and endLine: {evidence.path}" + else: + if evidence.start_line > evidence.end_line: + return f"evidence line range is inverted: {evidence.path}" + if evidence.end_line > _review_line_count(text): + return f"evidence line range exceeds file length: {evidence.path}" + if not _range_included(evidence.start_line, evidence.end_line, prompt_file): + return f"evidence line range was not included in review context: {evidence.path}" + if evidence.quote and evidence.quote.strip(): + target = prompt_file.included_text + if evidence.start_line is not None and evidence.end_line is not None: + target = "\n".join(text.splitlines()[evidence.start_line - 1 : evidence.end_line]) + if evidence.quote not in target and _compact_whitespace( + evidence.quote + ) not in _compact_whitespace(target): + return f"evidence quote does not match file contents: {evidence.path}" + return None + + +def _review_line_count(contents: str) -> int: + if contents == "": + return 1 + count = contents.count("\n") + return count if contents.endswith("\n") else count + 1 + + +def _range_included(start_line: int, end_line: int, prompt_file: Any) -> bool: + return any( + start_line >= line_range.start_line and end_line <= line_range.end_line + for line_range in prompt_file.included_line_ranges + ) + + +def _compact_whitespace(value: str) -> str: + return " ".join(value.split()) def _merge_review_findings( @@ -1034,6 +1149,8 @@ def _mark_feature_reviewed( run_id_value: str, *, provider: str, + manifest: ReviewPromptManifest, + dropped: int, ) -> None: all_ids = sorted({*feature.finding_ids, *finding_ids}) feature.finding_ids = all_ids @@ -1044,7 +1161,11 @@ def _mark_feature_reviewed( AnalysisEntry( run_id=run_id_value, kind="review", - summary=f"Reviewed with {len(finding_ids)} findings.", + summary=( + f"Reviewed with {len(finding_ids)} findings; dropped {dropped}; " + f"context {len(manifest.included_files)} files, " + f"~{manifest.approximate_tokens} tokens." + ), provider=provider, model=loaded.config.provider.model, reasoning_effort=loaded.config.provider.reasoning_effort, diff --git a/packages/pythinker-review/src/pythinker_review/security_scan/knowledge.py b/packages/pythinker-review/src/pythinker_review/security_scan/knowledge.py new file mode 100644 index 00000000..99830dd4 --- /dev/null +++ b/packages/pythinker-review/src/pythinker_review/security_scan/knowledge.py @@ -0,0 +1,1348 @@ +"""Shared security-review knowledge for prompts and advisor context. + +Most framework highlights and slug notes are ported from the TypeScript +``blackbox/pythinker-security-scanner`` prompt tables. Keep entries short: these are +reviewer instincts and false-positive checks, not tutorials. +""" + +from __future__ import annotations + +TechHighlight = tuple[str, tuple[str, ...], tuple[str, ...]] + +TECH_HIGHLIGHTS: dict[str, TechHighlight] = { + "actix": ( + "Actix-web", + ("rust",), + ( + "Middleware via `App::new().wrap(...)` is global; per-scope wraps via " + "`web::scope().wrap()` — flag scopes with skipped wraps", + "Extractors `web::Query` / `web::Json` / `web::Path` are user input — types " + "only validate STRUCTURE, not content", + "Auth middleware that returns `next.call(req)` unconditionally before the check is the " + "bypass shape", + '`HttpResponse::Ok().body(format!("{}", x))` is XSS — use a templating ' + "crate with escape", + ), + ), + "aiohttp": ( + "aiohttp", + ("python",), + ( + "Middleware via `@web.middleware` runs in declaration order — auth before logging is " + "the safe layout", + "`request.query` / `request.json()` / `request.match_info` / `request.read()` are " + "user input", + "`aiohttp_session` cookies need an explicit storage backend with secret rotation; " + "default `EncryptedCookieStorage` is fine", + "ClientSession (outbound) — flag user-controlled URLs without an allowlist (SSRF)", + ), + ), + "airflow": ( + "Airflow", + ("python",), + ( + "DAGs run with the Airflow scheduler's privileges — operator template fields (`{{ " + "params.x }}`) interpolated into Bash/SQL/HTTP are injection sinks", + '`BashOperator(bash_command=f"... {x}")` is shell injection — even non-templated ' + "f-strings are risky if x is user-influenced", + "Connections and Variables hold credentials — leaking them via XCom or logs is data " + "exposure", + "REST API auth (`auth_backends`) — defaults can be permissive on older versions", + ), + ), + "android": ( + "Android", + (), + ( + '`android:exported="true"` on Activity/Service/Receiver/Provider exposes the ' + "component to other apps — confirm with intent + permission", + "Implicit `` makes a component exported on pre-API-31 even without " + '`android:exported="true"` — flag legacy code', + 'Deeplink schemes (``) — review URL handling for SSRF ' + "(WebView), file:// loads, JS bridges", + "`WebView` with `setJavaScriptEnabled(true)` + `addJavascriptInterface` is RCE if " + "the loaded URL is attacker-controlled", + "ContentProvider exported without permission grants reads/writes to any app — " + '`android:grantUriPermissions="true"` widens scope further', + ), + ), + "apex": ( + "Apex (Salesforce)", + (), + ( + "`without sharing` classes BYPASS row-level security — confirm every `without sharing` " + "is intentional and that the methods can't be invoked by unprivileged users", + "`@AuraEnabled` methods are reachable from Lightning components without extra auth — " + "same surface as REST", + "`Database.query('SELECT ... WHERE ... = \\'' + userInput + '\\'')` is SOQL injection; " + "`[SELECT ... WHERE id = :userInput]` is bound and safe", + "FLS / CRUD checks (`Schema.sObjectType.X.isAccessible()`) are NOT automatic — flag DML " + "on sObjects without explicit checks", + "`@RestResource(urlMapping='...')` exposes the class on `/services/apexrest/` — public " + "to authenticated Salesforce users; confirm the data filter", + ), + ), + "astro": ( + "Astro", + ("typescript", "javascript"), + ( + "`pages/api/**/*.ts` exports (`GET`/`POST`/...) are public; `prerender = false` opts a " + "page into SSR with the same auth concerns", + "`Astro.request` / `Astro.cookies` / `Astro.params` are user input — same sinks as " + "Next.js", + "Default output is static; double-check if a route silently became SSR via `export " + "const prerender = false`", + "Astro uses Vite — env vars prefixed with `PUBLIC_` ship to the client bundle", + ), + ), + "aws-lambda": ( + "AWS Lambda", + (), + ( + "API Gateway authorizer claims live on `event.requestContext.authorizer` — " + "handlers that don't read them are unauthenticated", + "`event.body` is JSON-string in proxy integrations — JSON.parse failures should " + "NOT echo `event` (leaks request data into logs)", + "IAM role on the function determines blast radius — over-permissioned roles + RCE " + "= account takeover", + "Cold-start global state is shared across invocations on the same container — " + "credentials/PII can leak between tenants", + "Lambda timeouts default to 3s but can be 15min — long-running handlers without " + "per-call rate limits enable cost amplification", + ), + ), + "axum": ( + "Axum", + ("rust",), + ( + '`Router::new().route("/", get(h)).layer(auth_layer)` — `.layer` order matters; routes ' + "added AFTER `.layer` may not be wrapped", + "`Extension` / `State` carry auth identity — flag handlers that skip them", + "`Path` / `Query` / `Json` extractors are user input; same sinks as Actix", + "`.merge(other_router)` and `.nest(prefix, other)` — sub-routers inherit parent layers " + "but the order of `.layer` vs `.merge`/`.nest` matters", + ), + ), + "azure-functions": ( + "Azure Functions", + ("csharp", "javascript", "typescript", "python"), + ( + "`AuthorizationLevel.Anonymous` on `HttpTrigger` is a public endpoint — " + "confirm intent; `Function`/`Admin` require a function key", + "Function keys are NOT user identity — they authenticate the *caller app*, " + "not a user; for user auth use Easy Auth or App Service Authentication", + "Triggers (Queue/ServiceBus/Blob) are reached via Azure infra — payloads are " + "still user input if any web caller can write to the queue", + 'Bindings (e.g. `[Blob("path/{queueTrigger}")]`) interpolate input into ' + "resource paths — can be path traversal across containers", + ), + ), + "bottle": ( + "Bottle", + ("python",), + ( + "Bottle has no built-in auth — every `@route` is public unless a decorator chain " + "enforces a check", + "`request.query` / `request.forms` / `request.json` are user input", + "SimpleTemplate `{{!x}}` is unescaped; `{{x}}` auto-escapes — flag the bang form", + "`static_file(filename, root)` without `path.basename(filename)` is path traversal", + ), + ), + "buffalo": ( + "Buffalo", + ("go",), + ( + "`app.Use(...)` middleware is global; `app.Resource(...)` registers CRUD — confirm " + "auth wraps both", + "`c.Param('x')` / `c.Request()` / `c.Bind(&v)` are user input", + "`render.Auto` chooses HTML / JSON / XML by Accept header — DB rows in the response " + "include all columns; use a response shape", + ), + ), + "bullmq": ( + "BullMQ", + ("typescript", "javascript"), + ( + "`job.data` is whatever the producer enqueued — treat it as user input if any web " + "handler can enqueue", + "Workers run with elevated trust (no auth context) — confirm the queue boundary " + "validates / authorizes the request before enqueue", + "Retry on poison messages can amplify a single attacker payload across retries — flag " + "handlers without idempotency keys", + "`Queue.add(..., { delay })` at long delays plus user-controlled payload = " + "stored-XSS-via-job", + ), + ), + "bun": ( + "Bun", + ("typescript", "javascript"), + ( + "`Bun.serve({ fetch })` is a raw HTTP entry — auth/validation lives entirely in the " + "handler, no framework gates", + "`Bun.spawn(...)` / `Bun.$`...`` shell template — interpolated user input is RCE-shaped", + "Bun's TLS/HTTP defaults differ from Node; verify rejected-cert handling on outbound " + "`fetch`", + ), + ), + "cakephp": ( + "CakePHP", + ("php",), + ( + "`$this->Auth->allow(...)` opens specific actions to the public — confirm the list " + "is intentional", + "`$this->request->getData()` is user input; mass assignment via `patchEntity()` " + "without `accessibleFields` is the bug", + "Bake-generated views use `h($x)` for escape — flag templates that emit raw `$x` " + "without `h()`", + "`->find()->where(['col' => $x])` is parameterized; `->find()->where(\"col = " + "'$x'\")` is SQLi", + ), + ), + "celery": ( + "Celery", + ("python",), + ( + "Task args are deserialized via the configured serializer — `pickle` is unsafe " + "deserialization (RCE)", + "Tasks run with worker-level trust (no request user) — re-validate ownership when a " + "task acts on user data", + "`task.delay(user_id=...)` invocations from web code: confirm the call site " + "authenticates the user before enqueue", + "Long retries on poison messages can amplify a single bad payload", + ), + ), + "chi": ( + "Chi", + ("go",), + ( + "`r.Use(middleware)` and `r.Group(...)` define auth scopes — sub-routers inherit, but " + '`r.Mount("/x", h)` does NOT inherit middleware applied after the mount', + '`chi.URLParam(r, "id")` is user input; treat as untrusted in DB / fs / exec calls', + "`render.JSON(w, r, data)` returns whatever you pass — DB rows often include secret " + "columns; use a response-shape struct", + ), + ), + "clojure": ( + "Clojure (Ring/Compojure)", + (), + ( + "Ring middleware composes via `wrap-*`; auth must be in the chain BEFORE the route " + "handler", + "`wrap-anti-forgery` (CSRF) is opt-in — flag apps using session cookies without it", + 'Compojure `(GET "/x" [id] ...)` destructures params; `(get-in request [:params ' + ":x])` is the same — both untrusted", + "`ring.util.response/redirect` to user-controlled paths is open-redirect without an " + "allowlist", + ), + ), + "cobra": ( + "Cobra", + ("go",), + ( + "Privileged CLI surface — flags often hold secrets (`--token`, `--password`); flag any " + "logging of `cmd.Flags()`", + "`Run`/`RunE` handlers operate with the operator's privileges; user-supplied args " + "interpolated into shell or SQL are injection", + "`PersistentFlags` propagate to subcommands — credential flags on a parent leak to all " + "children", + ), + ), + "codeigniter": ( + "CodeIgniter", + ("php",), + ( + "Filters in `app/Config/Filters.php` are the auth gate; routes outside the " + "filter scope are public", + "`$this->request->getVar('x')` / `getPost()` are user input — concatenation into " + 'SQL via `$db->query("...$x...")` is injection', + "`view('name', $data)` auto-escapes; setting the third arg to disable escape " + "requires explicit trust review", + "`helper()` and `service()` calls can load arbitrary code if names are user-influenced", + ), + ), + "dart": ( + "Dart (Shelf)", + (), + ( + "Shelf has no built-in auth — `Pipeline().addMiddleware()` is the gate, registration " + "order matters", + "`request.url.queryParameters` / `request.readAsString()` are user input", + "`Response.ok(body)` doesn't HTML-escape; templates need explicit escape if rendering " + "HTML", + "`io.serve(handler, ...)` exposes the handler directly — no framework gates beyond what " + "you write", + ), + ), + "deno": ( + "Deno", + ("typescript", "javascript"), + ( + "`Deno.serve(handler)` is the entry; no built-in auth — middleware order is hand-rolled", + "Permissions (`--allow-net`, `--allow-read`, `--allow-env`) are deploy-time; code that " + "calls `Deno.permissions.request` at runtime is suspicious", + "Oak `ctx.request.body()` / `ctx.params` are untrusted; same sinks as Express", + ), + ), + "django": ( + "Django", + ("python",), + ( + "`@csrf_exempt` views handling state-changing POSTs without an alternate auth " + "(signature, token) are CSRF-vulnerable", + "`Model.objects.raw(...)` / `cursor.execute()` with f-string interpolation is SQL " + "injection — flag any %-formatted SQL", + "`mark_safe()` / `format_html()` on user input is XSS; same for `{% autoescape off " + "%}` blocks", + "`ModelForm` without `fields = [...]` (or with `__all__`) exposes mass-assignment of " + "every model column", + "`DEBUG=True` + `ALLOWED_HOSTS=['*']` in any reachable settings file leaks tracebacks " + "and SECRET_KEY material", + ), + ), + "djangorestframework": ( + "Django REST Framework", + ("python",), + ( + "`permission_classes` missing or set to `AllowAny` on a sensitive " + "`ModelViewSet` exposes full CRUD", + "`ModelSerializer` with `fields = '__all__'` allows mass-assignment of " + "admin-only columns via PATCH", + "`@action(detail=True)` methods inherit the viewset's permissions but " + "custom routers can break this — confirm", + ), + ), + "dotnet": ( + ".NET / ASP.NET Core", + ("csharp",), + ( + "`[Authorize]` is the gate; `[AllowAnonymous]` on a sensitive action opens it back up " + "— confirm intent", + "`[FromQuery]` / `[FromBody]` / `[FromRoute]` are user input — model binding is " + "structure-only", + "`[ApiController]` adds automatic 400 on model-state errors; absence means the " + "handler MUST check `ModelState.IsValid`", + "Razor `@Html.Raw(x)` on user input is XSS; bare `@x` HTML-encodes (safe)", + "Minimal API `app.MapGet(...).RequireAuthorization()` is the gate — flag chains " + "without it on sensitive routes", + 'EF Core `FromSqlRaw($"... {x} ...")` is SQLi; `FromSqlInterpolated($"... {x} ...")` ' + "parameterizes correctly", + ), + ), + "drupal": ( + "Drupal", + ("php",), + ( + "`*.routing.yml` `_permission`/`_access` keys are the gate — `access content` is " + "permissive (most authenticated users have it)", + "`\\Drupal::request()->query->get('x')` / `request->get('x')` are user input", + "`$this->t('@name', ['@name' => $userInput])` auto-escapes via `@`/`%`; bare " + "placeholders without prefix are unsafe", + "`db_query(\"... $x\")` is SQL injection; `\\Drupal::database()->query('... :x', " + "[':x' => $x])` is parameterized", + ), + ), + "echo": ( + "Echo", + ("go",), + ( + "`e.Use(middleware)` order matters — routes registered before `Use` aren't covered", + '`c.Bind(&v)` accepts JSON/form/query — fields with `json:"-"` matter only if you USE ' + '`json:"-"`; explicit allowlists in DTO structs are the safe form', + 'Group-level middleware (`g := e.Group("/api", auth)`) — confirm sensitive routes live ' + "under the group, not on the root `e`", + ), + ), + "erlang": ( + "Erlang (Cowboy)", + (), + ( + "`init/2` is the cowboy entry — auth check must happen before any state-changing call", + "`cowboy_req:binding(name, Req)` / `read_body/1` / `parse_qs/1` are user input", + "Erlang term decoding from external sources via `binary_to_term/1` is unsafe " + "deserialization — use `binary_to_term/2` with `[safe]`", + "Process-per-request model isolates handler crashes, but supervision-tree restart " + "strategies can hide errors", + ), + ), + "express": ( + "Express.js", + ("typescript", "javascript"), + ( + "Each `app.get/post/...` and `router.use` is a public endpoint — confirm auth " + "middleware actually wraps it (order matters; routes mounted before " + "`app.use(authMiddleware)` are unprotected)", + "`req.query`/`req.params`/`req.body` are user input; concatenation into SQL, shell, " + "paths, or URLs is the usual sink", + "`express.static` on a user-influenced root, or `res.sendFile(req.params.x)`, is " + "path traversal", + "Error handlers that send `err.stack` or `err.message` to the response leak internals", + "CORS `origin: true` reflecting credentials enables CSRF-via-fetch", + ), + ), + "falcon": ( + "Falcon", + ("python",), + ( + "`on_(self, req, resp, ...)` handlers are public unless a middleware/hook " + "checks auth", + "`req.media` / `req.params` / `req.get_param('x')` are user input", + "`req.context` carries auth-claim data — confirm it's set BEFORE the resource handler " + "runs", + "Falcon's `resp.media` accepts dicts directly; over-fetching DB rows leaks PII", + ), + ), + "fastapi": ( + "FastAPI", + ("python",), + ( + "Auth lives in `Depends(...)`; routes without an auth dependency are public — " + "`@app.get('/admin')` with no Depends is the common gap", + "Pydantic models validate input but `Optional[Any]` / `dict` fields are an escape " + "hatch — flag them on inputs", + "`response_model=...` filters server output; without it, you may return DB columns " + "containing secrets", + "`StaticFiles(directory=...)` rooted at a user-influenced path is path traversal", + ), + ), + "fastify": ( + "Fastify", + ("typescript", "javascript"), + ( + "`preHandler` / `onRequest` hooks are the auth layer; routes registered without them " + "or before the auth plugin are unprotected", + "Schema validation (`schema: { body, querystring }`) is the default mitigation — " + "flag handlers that read raw `request.body` without a schema", + "Plugins registered with `register()` inherit hooks per-scope; cross-scope auth " + "bypass is common in monorepos", + "`reply.send(err)` returns full error objects in dev mode; check the prod config", + ), + ), + "fiber": ( + "Fiber", + ("go",), + ( + "`app.Get/Post/...` registers public endpoints; middleware via `app.Use(auth)` must " + "precede them, and route-level middleware override group middleware", + "`c.Query` / `c.Params` / `c.Body` / `c.BodyParser(&v)` are user input; injection " + "sinks are the same as net/http", + "Fiber wraps fasthttp — request bodies/headers are not safe to retain past the handler " + "return; flag goroutines that capture `c` by reference", + ), + ), + "flask": ( + "Flask", + ("python",), + ( + "`@app.route(...)` without a `@login_required` (or equivalent) decorator is public; " + "check the order of decorators — `@app.route` must be outermost", + "`render_template_string(user_input)` is server-side template injection (RCE)", + "`request.args` / `request.form` / `request.json` interpolated into SQL via " + '`db.engine.execute(f"...")` is SQL injection', + "`send_from_directory(dir, request.args['file'])` without a basename check is path " + "traversal", + "`session` cookies use `app.secret_key` — hardcoded keys in source are session forgery", + ), + ), + "gcp-cloud-functions": ( + "GCP Cloud Functions", + (), + ( + "Allow-unauthenticated invocations (`--allow-unauthenticated`) make the " + "function public — confirm via deploy config", + "IAM-based auth (Cloud IAM) is invoker-level; for user identity, " + "integrate Identity Platform / Firebase Auth in the handler", + "Function URLs include the project ID and region — leakage of these is " + "information disclosure", + "Background functions (Pub/Sub, Storage triggers) — payload comes from " + "the GCP infra but is still ATTACKER-INFLUENCED if any web path can " + "write to the bucket/topic", + ), + ), + "gin": ( + "Gin", + ("go",), + ( + "Each `r.GET/POST/...` and `r.Group(...)` is a public endpoint; auth middleware applied " + "via `r.Use(...)` must precede route registration in the same group", + "`c.Query`/`c.Param`/`c.PostForm` are user input — usual injection surfaces (SQL, exec, " + "fs, URL) apply", + '`c.HTML(http.StatusOK, "tmpl", data)` with `data` containing untrusted strings is XSS ' + "unless the template uses `{{.X}}` (auto-escaped) and not `{{.X | safehtml}}`", + ), + ), + "github-actions": ( + "GitHub Actions", + ("yaml", "yml"), + ( + "pull_request_target and workflow_run can expose secrets to untrusted code.", + "Actions should be pinned; github.event/head_ref interpolation in run scripts " + "is shell-injection shaped.", + "permissions: write-all and broad id-token: write need justification.", + ), + ), + "go": ( + "Go web services", + ("go",), + ( + "Router middleware must wrap the exact route/group before registration.", + "c.Query/r.URL.Query/FormValue/path params are untrusted for SQL, exec, fs, " + "and HTTP clients.", + "Prefer response-shape structs to raw DB rows.", + ), + ), + "gorilla": ( + "Gorilla mux", + ("go",), + ( + "`router.Use(authMiddleware)` covers the router; subrouters via `Subrouter()` " + "inherit, but `PathPrefix(...).Handler(other)` does not", + "`mux.Vars(r)` is user input — usual injection sinks (SQL, exec, fs, URL)", + '`router.HandleFunc("/x", h).Methods("GET")` — flag handlers without an explicit ' + "`.Methods` (accept any verb)", + ), + ), + "grape": ( + "Grape", + ("ruby",), + ( + "Auth lives in `before do ... end` or `helpers do ... end` — endpoints without it are " + "public", + "`params` is the only safe input accessor; raw `request.body.read` skips Grape's " + "coercion", + "`declared(params, include_missing: false)` is strong-params equivalent — flag " + "handlers that use `params` directly for mass assignment", + "API versioning paths (`version 'v1'`) — confirm deprecated versions still enforce " + "auth", + ), + ), + "graphql": ( + "GraphQL", + ("typescript", "javascript"), + ( + "Per-resolver auth: every Query/Mutation/Subscription field is independently " + "reachable — flag resolvers that don't check `context.user`", + "Field-level vs object-level auth: returning a User object grants access to all " + "fields unless guards exist on `email`/`role`/etc.", + "Disabled introspection in prod — leaving it on leaks the full schema (informational " + "severity)", + "Query depth/complexity limits stop abusive nested queries; absence is the bug", + "Aliasing + batching can multiply the cost of an unauthenticated query — expensive " + "resolvers need rate limits", + ), + ), + "hanami": ( + "Hanami", + ("ruby",), + ( + "Each `Hanami::Action` subclass is publicly addressable via the router — `before` " + "callbacks are the auth gate", + "Strong-params equivalent: `params.valid?` + a Contract — handlers using raw `params` " + "skip validation", + "`include Deps[...]` for DI: shared DB / repo objects can leak ownership semantics if " + "used as singletons", + ), + ), + "hapi": ( + "Hapi", + ("typescript", "javascript"), + ( + "`auth: false` on a `server.route(...)` opts out of the default auth strategy — confirm " + "it's intentional, especially on writes", + "Validate routes use `validate: { query, payload, params }`; routes without validation " + "pass raw input to handlers", + "`server.auth.default(...)` sets the global gate; flag handlers that pre-date it or " + "that pass `auth: 'optional'`", + "`request.payload` / `request.query` / `request.params` are user input", + ), + ), + "hono": ( + "Hono", + ("typescript", "javascript"), + ( + "Each `app.get('/path', handler)` is a public endpoint; auth middleware (`app.use('*', " + "auth)`) must come BEFORE the route declarations or it's a no-op", + "`c.req.query()` / `c.req.param()` / `c.req.json()` are user input — usual injection " + "surfaces apply", + "Hono runs on Workers/edge runtimes — check whether route handlers reach into a " + "separate Node backend without re-authenticating", + ), + ), + "ios": ( + "iOS", + (), + ( + "`CFBundleURLSchemes` registers your app as a URL handler — `application(_:open:)` / " + "`scene(_:openURLContexts:)` receive attacker-controlled URLs", + "Universal Links via `apple-app-site-association` — host association determines which " + "domains can open the app; misconfig is account-takeover-shaped", + "WKWebView with `loadHTMLString(html, baseURL:)` and a `file://` baseURL gives the page " + "access to local files", + "Keychain access without `kSecAttrAccessibleWhenUnlocked` (or stricter) leaks " + "credentials at app launch", + "App Transport Security exceptions in Info.plist (`NSAllowsArbitraryLoads`) downgrade " + "TLS — flag any plist that opts out", + ), + ), + "jaxrs": ( + "JAX-RS (Jersey/Quarkus/RESTEasy)", + ("java", "kotlin"), + ( + "`@RolesAllowed`/`@DenyAll`/`@PermitAll` are the gate; absence on a `@Path` resource " + "is public", + "`@QueryParam`/`@PathParam`/`@FormParam`/`@HeaderParam` are user input", + "`@RequestScoped` provider classes can leak per-request state if held by " + "`@ApplicationScoped` resources", + "`Response.ok(entity)` with raw JPA entities over-fetches columns; use a DTO", + ), + ), + "kemal": ( + "Crystal Kemal", + (), + ( + "No built-in auth — every `get '/path' do ... end` is public unless a `before_*` " + "filter intercepts", + '`env.params.url["x"]` / `env.params.json["x"]` / `env.params.body["x"]` are user ' + "input", + "Crystal's macro-driven JSON parsing is type-safe but content-unvalidated; bounds " + "checks on collections matter", + ), + ), + "koa": ( + "Koa", + ("typescript", "javascript"), + ( + "`router.` routes registered before `app.use(authMiddleware)` are unprotected — " + "middleware order matters", + "`ctx.request.body` / `ctx.query` / `ctx.params` are user input; same injection sinks as " + "Express", + "`ctx.throw(401)` is a soft response — confirm it's reached BEFORE any data is " + "fetched/returned", + "`koa-bodyparser` defaults to forms+json; large payload limits and prototype-pollution " + "opts must be set explicitly", + ), + ), + "ktor": ( + "Ktor", + ("kotlin",), + ( + '`authenticate("jwt") { ... }` blocks are the gate — routes outside them are public', + "`call.receive()` deserializes user input — `kotlinx.serialization` is " + "structure-validating, not content-validating", + "`call.parameters` / `call.request.queryParameters` are user input", + "Status pages plugin handles errors — confirm prod config doesn't echo exceptions to " + "the response", + ), + ), + "lambda-rs": ( + "Rust AWS Lambda", + ("rust",), + ( + "`LambdaEvent::payload` is API Gateway / SQS / etc. payload — type-driven but " + "content is user-supplied", + "`event.payload.request_context.authorizer` carries claims when API Gateway " + "authorizer is configured — handler must verify", + "Cold-start global state (lazy_static / OnceCell) survives across invocations — " + "credentials/state leakage between tenants", + ), + ), + "laravel": ( + "Laravel", + ("php",), + ( + "`Model::create($request->all())` without `$fillable`/`$guarded` is mass assignment " + "— admin columns get overwritten", + "`DB::raw()` / `whereRaw()` / `selectRaw()` with interpolated input is SQL injection", + "`VerifyCsrfToken::$except` lists that include state-changing routes are " + "CSRF-vulnerable unless an alternate verification (signed URL, webhook signature) " + "exists", + "Blade `{!! $x !!}` renders raw HTML — XSS sink", + "Routes outside the `auth` middleware group, or routes with " + "`->withoutMiddleware([...])`, need explicit per-action auth checks", + ), + ), + "magento": ( + "Magento", + ("php",), + ( + "ACL via `etc/acl.xml`; webapi routes via `etc/webapi.xml` `` — flag " + "routes set to `anonymous` doing sensitive work", + "`$this->getRequest()->getParam('x')` is user input", + "Plugin/observer code runs in core context — privilege escalation is easy if input " + "isn't sanitized", + "Customer data via `\\Magento\\Customer\\Api` requires customer ID; flag any read " + "using user-supplied ID without ownership check", + ), + ), + "mcp": ( + "MCP / agentic tools", + ("typescript", "javascript", "python"), + ( + "Tool inputs and retrieved content are untrusted data, not instructions.", + "Tool schemas need allowlists, execution caps, and explicit filesystem/network " + "boundaries.", + ), + ), + "micronaut": ( + "Micronaut", + ("java", "kotlin"), + ( + "`@Secured(SecurityRule.IS_AUTHENTICATED)` on controller is the gate; `@PermitAll` " + "opens it back up", + "`@Body` / `@QueryValue` / `@PathVariable` are user input", + "Reactive endpoints return `Mono`/`Flux` — auth check must be in the reactive " + "chain, not just the handler signature", + "Bean introspection (compile-time DI) means runtime config can't easily swap auth " + "— flag config-driven gates", + ), + ), + "nestjs": ( + "NestJS", + ("typescript", "javascript"), + ( + "`@UseGuards(...)` on controller or method is the auth check; missing guards on a " + "`@Controller()` are a common gap", + "`@Body()` / `@Query()` without a `class-validator` DTO is unvalidated input", + "Global pipes/interceptors registered late or only in main.ts may not apply to " + "e2e-test routes shipped to prod", + "`@Public()` decorators that opt OUT of a global auth guard — confirm they are " + "intentional", + ), + ), + "nextjs": ( + "Next.js", + ("typescript", "javascript"), + ( + "Next.js `middleware.ts` runs at the edge and is NOT sufficient auth — too easy to " + "misconfigure or bypass via routes that escape the matcher", + "Server Actions are publicly callable POST endpoints — every one needs explicit auth " + "+ authorization checks", + "`JSON.stringify()` inside `dangerouslySetInnerHTML` or inline `