surshar / claude-code
Claude Code is an agentic coding tool that lives in your terminal, understands your codebase, and helps you code faster by executing routine tasks, explaining complex code, and handling git workflows - all through natural language commands.
Built by Anthropic · github.com/anthropics
Replace the plugin directory with the current copy from anthropics/claude-plugins-official and bring the marketplace entry (version, description) and the plugins README row in line with it.
| @@ -137,8 +137,8 @@ | ||
| 137 | 137 | }, |
| 138 | 138 | { |
| 139 | 139 | "name": "security-guidance", |
| 140 | - "description": "Security reminder hook that warns about potential security issues when editing files, including command injection, XSS, and unsafe code patterns", | |
| 141 | - "version": "1.0.0", | |
| 140 | + "description": "Security review for Claude-generated code. Pattern-based warnings on edits, LLM-powered diff review on Stop, and an agentic commit reviewer that catches injection, XSS, SSRF, hardcoded secrets, and 25+ other vulnerability classes.", | |
| 141 | + "version": "2.0.12", | |
| 142 | 142 | "author": { |
| 143 | 143 | "name": "David Dworken", |
| 144 | 144 | "email": "dworken@anthropic.com" |
| @@ -24,7 +24,7 @@ | ||
| 24 | 24 | | [plugin-dev](./plugin-dev/) | Comprehensive toolkit for developing Claude Code plugins with 7 expert skills and AI-assisted creation | **Command:** `/plugin-dev:create-plugin` - 8-phase guided workflow for building plugins<br>**Agents:** `agent-creator`, `plugin-validator`, `skill-reviewer`<br>**Skills:** Hook development, MCP integration, plugin structure, settings, commands, agents, and skill development | |
| 25 | 25 | | [pr-review-toolkit](./pr-review-toolkit/) | Comprehensive PR review agents specializing in comments, tests, error handling, type design, code quality, and code simplification | **Command:** `/pr-review-toolkit:review-pr` - Run with optional review aspects (comments, tests, errors, types, code, simplify, all)<br>**Agents:** `comment-analyzer`, `pr-test-analyzer`, `silent-failure-hunter`, `type-design-analyzer`, `code-reviewer`, `code-simplifier` | |
| 26 | 26 | | [ralph-wiggum](./ralph-wiggum/) | Interactive self-referential AI loops for iterative development. Claude works on the same task repeatedly until completion | **Commands:** `/ralph-loop`, `/cancel-ralph` - Start/stop autonomous iteration loops<br>**Hook:** Stop - Intercepts exit attempts to continue iteration | |
| 27 | -| [security-guidance](./security-guidance/) | Security reminder hook that warns about potential security issues when editing files | **Hook:** PreToolUse - Monitors 9 security patterns including command injection, XSS, eval usage, dangerous HTML, pickle deserialization, and os.system calls | | |
| 27 | +| [security-guidance](./security-guidance/) | Security review for Claude-generated code: pattern-based warnings on edits, LLM-powered diff review on Stop, and an agentic commit reviewer | **Hooks:** PostToolUse - Pattern warnings on file edits and review of `git commit` / `git push`<br>**Hook:** Stop - Reviews the turn's diff for vulnerabilities<br>**Hooks:** SessionStart, UserPromptSubmit - Setup and per-turn state | | |
| 28 | 28 | |
| 29 | 29 | ## Installation |
| 30 | 30 |
| @@ -1,10 +1,10 @@ | ||
| 1 | 1 | { |
| 2 | 2 | "name": "security-guidance", |
| 3 | - "version": "2.0.0", | |
| 3 | + "version": "2.0.12", | |
| 4 | 4 | "description": "Security review for Claude-generated code. Pattern-based warnings on edits, LLM-powered diff review on Stop, and an agentic commit reviewer that catches injection, XSS, SSRF, hardcoded secrets, and 25+ other vulnerability classes.", |
| 5 | 5 | "author": { |
| 6 | 6 | "name": "David Dworken", |
| 7 | 7 | "email": "dworken@anthropic.com" |
| 8 | 8 | }, |
| 9 | - "homepage": "https://github.com/anthropics/claude-code/tree/main/plugins/security-guidance" | |
| 9 | + "homepage": "https://github.com/anthropics/claude-plugins-official/tree/main/plugins/security-guidance" | |
| 10 | 10 | } |
| @@ -1,0 +1,202 @@ | ||
| 1 | + | |
| 2 | + Apache License | |
| 3 | + Version 2.0, January 2004 | |
| 4 | + http://www.apache.org/licenses/ | |
| 5 | + | |
| 6 | + TERMS AND CONDITIONS FOR USE, REPRODUCTION, AND DISTRIBUTION | |
| 7 | + | |
| 8 | + 1. Definitions. | |
| 9 | + | |
| 10 | + "License" shall mean the terms and conditions for use, reproduction, | |
| 11 | + and distribution as defined by Sections 1 through 9 of this document. | |
| 12 | + | |
| 13 | + "Licensor" shall mean the copyright owner or entity authorized by | |
| 14 | + the copyright owner that is granting the License. | |
| 15 | + | |
| 16 | + "Legal Entity" shall mean the union of the acting entity and all | |
| 17 | + other entities that control, are controlled by, or are under common | |
| 18 | + control with that entity. For the purposes of this definition, | |
| 19 | + "control" means (i) the power, direct or indirect, to cause the | |
| 20 | + direction or management of such entity, whether by contract or | |
| 21 | + otherwise, or (ii) ownership of fifty percent (50%) or more of the | |
| 22 | + outstanding shares, or (iii) beneficial ownership of such entity. | |
| 23 | + | |
| 24 | + "You" (or "Your") shall mean an individual or Legal Entity | |
| 25 | + exercising permissions granted by this License. | |
| 26 | + | |
| 27 | + "Source" form shall mean the preferred form for making modifications, | |
| 28 | + including but not limited to software source code, documentation | |
| 29 | + source, and configuration files. | |
| 30 | + | |
| 31 | + "Object" form shall mean any form resulting from mechanical | |
| 32 | + transformation or translation of a Source form, including but | |
| 33 | + not limited to compiled object code, generated documentation, | |
| 34 | + and conversions to other media types. | |
| 35 | + | |
| 36 | + "Work" shall mean the work of authorship, whether in Source or | |
| 37 | + Object form, made available under the License, as indicated by a | |
| 38 | + copyright notice that is included in or attached to the work | |
| 39 | + (an example is provided in the Appendix below). | |
| 40 | + | |
| 41 | + "Derivative Works" shall mean any work, whether in Source or Object | |
| 42 | + form, that is based on (or derived from) the Work and for which the | |
| 43 | + editorial revisions, annotations, elaborations, or other modifications | |
| 44 | + represent, as a whole, an original work of authorship. For the purposes | |
| 45 | + of this License, Derivative Works shall not include works that remain | |
| 46 | + separable from, or merely link (or bind by name) to the interfaces of, | |
| 47 | + the Work and Derivative Works thereof. | |
| 48 | + | |
| 49 | + "Contribution" shall mean any work of authorship, including | |
| 50 | + the original version of the Work and any modifications or additions | |
| 51 | + to that Work or Derivative Works thereof, that is intentionally | |
| 52 | + submitted to Licensor for inclusion in the Work by the copyright owner | |
| 53 | + or by an individual or Legal Entity authorized to submit on behalf of | |
| 54 | + the copyright owner. For the purposes of this definition, "submitted" | |
| 55 | + means any form of electronic, verbal, or written communication sent | |
| 56 | + to the Licensor or its representatives, including but not limited to | |
| 57 | + communication on electronic mailing lists, source code control systems, | |
| 58 | + and issue tracking systems that are managed by, or on behalf of, the | |
| 59 | + Licensor for the purpose of discussing and improving the Work, but | |
| 60 | + excluding communication that is conspicuously marked or otherwise | |
| 61 | + designated in writing by the copyright owner as "Not a Contribution." | |
| 62 | + | |
| 63 | + "Contributor" shall mean Licensor and any individual or Legal Entity | |
| 64 | + on behalf of whom a Contribution has been received by Licensor and | |
| 65 | + subsequently incorporated within the Work. | |
| 66 | + | |
| 67 | + 2. Grant of Copyright License. Subject to the terms and conditions of | |
| 68 | + this License, each Contributor hereby grants to You a perpetual, | |
| 69 | + worldwide, non-exclusive, no-charge, royalty-free, irrevocable | |
| 70 | + copyright license to reproduce, prepare Derivative Works of, | |
| 71 | + publicly display, publicly perform, sublicense, and distribute the | |
| 72 | + Work and such Derivative Works in Source or Object form. | |
| 73 | + | |
| 74 | + 3. Grant of Patent License. Subject to the terms and conditions of | |
| 75 | + this License, each Contributor hereby grants to You a perpetual, | |
| 76 | + worldwide, non-exclusive, no-charge, royalty-free, irrevocable | |
| 77 | + (except as stated in this section) patent license to make, have made, | |
| 78 | + use, offer to sell, sell, import, and otherwise transfer the Work, | |
| 79 | + where such license applies only to those patent claims licensable | |
| 80 | + by such Contributor that are necessarily infringed by their | |
| 81 | + Contribution(s) alone or by combination of their Contribution(s) | |
| 82 | + with the Work to which such Contribution(s) was submitted. If You | |
| 83 | + institute patent litigation against any entity (including a | |
| 84 | + cross-claim or counterclaim in a lawsuit) alleging that the Work | |
| 85 | + or a Contribution incorporated within the Work constitutes direct | |
| 86 | + or contributory patent infringement, then any patent licenses | |
| 87 | + granted to You under this License for that Work shall terminate | |
| 88 | + as of the date such litigation is filed. | |
| 89 | + | |
| 90 | + 4. Redistribution. You may reproduce and distribute copies of the | |
| 91 | + Work or Derivative Works thereof in any medium, with or without | |
| 92 | + modifications, and in Source or Object form, provided that You | |
| 93 | + meet the following conditions: | |
| 94 | + | |
| 95 | + (a) You must give any other recipients of the Work or | |
| 96 | + Derivative Works a copy of this License; and | |
| 97 | + | |
| 98 | + (b) You must cause any modified files to carry prominent notices | |
| 99 | + stating that You changed the files; and | |
| 100 | + | |
| 101 | + (c) You must retain, in the Source form of any Derivative Works | |
| 102 | + that You distribute, all copyright, patent, trademark, and | |
| 103 | + attribution notices from the Source form of the Work, | |
| 104 | + excluding those notices that do not pertain to any part of | |
| 105 | + the Derivative Works; and | |
| 106 | + | |
| 107 | + (d) If the Work includes a "NOTICE" text file as part of its | |
| 108 | + distribution, then any Derivative Works that You distribute must | |
| 109 | + include a readable copy of the attribution notices contained | |
| 110 | + within such NOTICE file, excluding those notices that do not | |
| 111 | + pertain to any part of the Derivative Works, in at least one | |
| 112 | + of the following places: within a NOTICE text file distributed | |
| 113 | + as part of the Derivative Works; within the Source form or | |
| 114 | + documentation, if provided along with the Derivative Works; or, | |
| 115 | + within a display generated by the Derivative Works, if and | |
| 116 | + wherever such third-party notices normally appear. The contents | |
| 117 | + of the NOTICE file are for informational purposes only and | |
| 118 | + do not modify the License. You may add Your own attribution | |
| 119 | + notices within Derivative Works that You distribute, alongside | |
| 120 | + or as an addendum to the NOTICE text from the Work, provided | |
| 121 | + that such additional attribution notices cannot be construed | |
| 122 | + as modifying the License. | |
| 123 | + | |
| 124 | + You may add Your own copyright statement to Your modifications and | |
| 125 | + may provide additional or different license terms and conditions | |
| 126 | + for use, reproduction, or distribution of Your modifications, or | |
| 127 | + for any such Derivative Works as a whole, provided Your use, | |
| 128 | + reproduction, and distribution of the Work otherwise complies with | |
| 129 | + the conditions stated in this License. | |
| 130 | + | |
| 131 | + 5. Submission of Contributions. Unless You explicitly state otherwise, | |
| 132 | + any Contribution intentionally submitted for inclusion in the Work | |
| 133 | + by You to the Licensor shall be under the terms and conditions of | |
| 134 | + this License, without any additional terms or conditions. | |
| 135 | + Notwithstanding the above, nothing herein shall supersede or modify | |
| 136 | + the terms of any separate license agreement you may have executed | |
| 137 | + with Licensor regarding such Contributions. | |
| 138 | + | |
| 139 | + 6. Trademarks. This License does not grant permission to use the trade | |
| 140 | + names, trademarks, service marks, or product names of the Licensor, | |
| 141 | + except as required for reasonable and customary use in describing the | |
| 142 | + origin of the Work and reproducing the content of the NOTICE file. | |
| 143 | + | |
| 144 | + 7. Disclaimer of Warranty. Unless required by applicable law or | |
| 145 | + agreed to in writing, Licensor provides the Work (and each | |
| 146 | + Contributor provides its Contributions) on an "AS IS" BASIS, | |
| 147 | + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or | |
| 148 | + implied, including, without limitation, any warranties or conditions | |
| 149 | + of TITLE, NON-INFRINGEMENT, MERCHANTABILITY, or FITNESS FOR A | |
| 150 | + PARTICULAR PURPOSE. You are solely responsible for determining the | |
| 151 | + appropriateness of using or redistributing the Work and assume any | |
| 152 | + risks associated with Your exercise of permissions under this License. | |
| 153 | + | |
| 154 | + 8. Limitation of Liability. In no event and under no legal theory, | |
| 155 | + whether in tort (including negligence), contract, or otherwise, | |
| 156 | + unless required by applicable law (such as deliberate and grossly | |
| 157 | + negligent acts) or agreed to in writing, shall any Contributor be | |
| 158 | + liable to You for damages, including any direct, indirect, special, | |
| 159 | + incidental, or consequential damages of any character arising as a | |
| 160 | + result of this License or out of the use or inability to use the | |
| 161 | + Work (including but not limited to damages for loss of goodwill, | |
| 162 | + work stoppage, computer failure or malfunction, or any and all | |
| 163 | + other commercial damages or losses), even if such Contributor | |
| 164 | + has been advised of the possibility of such damages. | |
| 165 | + | |
| 166 | + 9. Accepting Warranty or Additional Liability. While redistributing | |
| 167 | + the Work or Derivative Works thereof, You may choose to offer, | |
| 168 | + and charge a fee for, acceptance of support, warranty, indemnity, | |
| 169 | + or other liability obligations and/or rights consistent with this | |
| 170 | + License. However, in accepting such obligations, You may act only | |
| 171 | + on Your own behalf and on Your sole responsibility, not on behalf | |
| 172 | + of any other Contributor, and only if You agree to indemnify, | |
| 173 | + defend, and hold each Contributor harmless for any liability | |
| 174 | + incurred by, or claims asserted against, such Contributor by reason | |
| 175 | + of your accepting any such warranty or additional liability. | |
| 176 | + | |
| 177 | + END OF TERMS AND CONDITIONS | |
| 178 | + | |
| 179 | + APPENDIX: How to apply the Apache License to your work. | |
| 180 | + | |
| 181 | + To apply the Apache License to your work, attach the following | |
| 182 | + boilerplate notice, with the fields enclosed by brackets "[]" | |
| 183 | + replaced with your own identifying information. (Don't include | |
| 184 | + the brackets!) The text should be enclosed in the appropriate | |
| 185 | + comment syntax for the file format. We also recommend that a | |
| 186 | + file or class name and description of purpose be included on the | |
| 187 | + same "printed page" as the copyright notice for easier | |
| 188 | + identification within third-party archives. | |
| 189 | + | |
| 190 | + Copyright [yyyy] [name of copyright owner] | |
| 191 | + | |
| 192 | + Licensed under the Apache License, Version 2.0 (the "License"); | |
| 193 | + you may not use this file except in compliance with the License. | |
| 194 | + You may obtain a copy of the License at | |
| 195 | + | |
| 196 | + http://www.apache.org/licenses/LICENSE-2.0 | |
| 197 | + | |
| 198 | + Unless required by applicable law or agreed to in writing, software | |
| 199 | + distributed under the License is distributed on an "AS IS" BASIS, | |
| 200 | + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | |
| 201 | + See the License for the specific language governing permissions and | |
| 202 | + limitations under the License. |
| @@ -3,7 +3,7 @@ | ||
| 3 | 3 | Security review for Claude-generated code. Three layers: |
| 4 | 4 | |
| 5 | 5 | 1. **Pattern warnings** — instant regex-based reminders on `Edit`/`Write` for ~25 known-dangerous patterns (`yaml.load`, `torch.load(weights_only=False)`, `pickle.load` on untrusted data, raw `innerHTML`, hardcoded secrets, etc.). |
| 6 | -2. **LLM diff review** — when Claude finishes a turn, the plugin sends the diff to a fast LLM call (Opus 4.7 by default) and feeds high-severity findings back to Claude so it can fix them before you see the response. | |
| 6 | +2. **LLM diff review** — when Claude finishes a turn, the plugin sends the diff to a fast LLM call (the newest Opus by default) and feeds high-severity findings back to Claude so it can fix them before you see the response. | |
| 7 | 7 | 3. **Agentic commit review** — on `git commit`, an SDK-driven reviewer reads related files (`Read`/`Grep`/`Glob`) to trace data flow across the codebase, catching multi-file vulnerabilities pattern matching misses (IDOR, auth bypass, cross-file SSRF). |
| 8 | 8 | |
| 9 | 9 | Findings cover common web-vulnerability classes — injection, XSS, SSRF, hardcoded secrets, IDOR, auth bypass, unsafe deserialization, and path traversal among others. |
| @@ -28,18 +28,25 @@ | ||
| 28 | 28 | |
| 29 | 29 | ### Selecting a model |
| 30 | 30 | |
| 31 | +By default both reviews run on the newest Claude Opus, and move to the next one when it is released — there is no model version to bump: | |
| 32 | + | |
| 33 | +- The LLM diff review asks your API endpoint (`GET /v1/models`) which Opus models it serves and uses the newest. The answer is cached for a day in the plugin's state directory. If the endpoint can't answer, the review uses `claude-opus-5-5`. | |
| 34 | +- The agentic commit reviewer, and every review on Bedrock, Vertex, and Foundry, runs through your installed Claude Code and uses its `opus` alias — the same model `/model opus` selects, including any `ANTHROPIC_DEFAULT_OPUS_MODEL` you have set. | |
| 35 | + | |
| 36 | +To pin a model, or to use a cheaper one: | |
| 37 | + | |
| 31 | 38 | ```bash |
| 32 | 39 | # 1P / gateway: a canonical model id |
| 33 | -SECURITY_REVIEW_MODEL=claude-opus-4-7 # default | |
| 40 | +SECURITY_REVIEW_MODEL=claude-opus-5-5 | |
| 34 | 41 | |
| 35 | 42 | # Bedrock: use the inference-profile id |
| 36 | -SECURITY_REVIEW_MODEL=us.anthropic.claude-opus-4-7 | |
| 43 | +SECURITY_REVIEW_MODEL=us.anthropic.claude-opus-5-5 | |
| 37 | 44 | |
| 38 | -# Vertex: use the Vertex date-tag form | |
| 39 | -SECURITY_REVIEW_MODEL=claude-opus-4-7@20260218 | |
| 45 | +# Vertex / Foundry: the provider's model id | |
| 46 | +SECURITY_REVIEW_MODEL=claude-opus-5-5 | |
| 40 | 47 | ``` |
| 41 | 48 | |
| 42 | -`SECURITY_REVIEW_MODEL` controls the LLM diff review. `SG_AGENTIC_MODEL` (same syntax) controls the agentic commit reviewer; defaults to the same model. | |
| 49 | +`SECURITY_REVIEW_MODEL` controls the LLM diff review. `SG_AGENTIC_MODEL` (same syntax) controls the agentic commit reviewer. | |
| 43 | 50 | |
| 44 | 51 | ### Enabling/disabling layers |
| 45 | 52 | |
| @@ -101,9 +108,9 @@ | ||
| 101 | 108 | |
| 102 | 109 | **Plugin doesn't seem to fire** — check that `~/.claude/claude-security-guidance.md` (or hook activity) shows in debug logs. Run Claude Code with `--debug-file /tmp/claude/debug.txt` and grep for `security_reminder_hook`. The plugin also writes its own log to `~/.claude/security/log.txt`. |
| 103 | 110 | |
| 104 | -**Review never finds anything** — verify your API path works. On 3P providers, check `SECURITY_REVIEW_MODEL` is set to a provider-specific id (not a bare `claude-opus-4-7`). On LLM gateways, check the gateway's logs for `POST /v1/messages` traffic from the plugin. | |
| 111 | +**Review never finds anything** — verify your API path works. On 3P providers, if you set `SECURITY_REVIEW_MODEL`, check it is a provider-specific id your account can invoke. On LLM gateways, check the gateway's logs for `POST /v1/messages` traffic from the plugin. | |
| 105 | 112 | |
| 106 | -**Too many false positives** — drop `SECURITY_REVIEW_MODEL` to a cheaper model (`claude-sonnet-4-6`) and re-evaluate; if precision is the priority, stay on Opus 4.7. | |
| 113 | +**Too many false positives** — set `SECURITY_REVIEW_MODEL` to a cheaper model (`claude-sonnet-5-5`) and re-evaluate; if precision is the priority, stay on the default. | |
| 107 | 114 | |
| 108 | 115 | **Want to silence a specific finding** — add a comment to the line explaining why it's safe; the LLM reviewer treats inline justifications as exclusions. For systemic exclusions, document them in your `claude-security-guidance.md`. |
| 109 | 116 |
| @@ -10,15 +10,42 @@ | ||
| 10 | 10 | import threading |
| 11 | 11 | from datetime import datetime |
| 12 | 12 | |
| 13 | +def state_dir(): | |
| 14 | + """Return the absolute path of the plugin's state directory. | |
| 15 | + | |
| 16 | + Resolution precedence (highest first): | |
| 17 | + 1. SECURITY_WARNINGS_STATE_DIR — plugin-specific override (existing) | |
| 18 | + 2. CLAUDE_CONFIG_DIR/security — CC's config-dir env var (#1868) | |
| 19 | + 3. ~/.claude/security — default fallback | |
| 20 | + | |
| 21 | + Empty-string env vars are treated as not-set so a misconfigured shell | |
| 22 | + (`CLAUDE_CONFIG_DIR=` with no value) doesn't silently write to | |
| 23 | + /security at the filesystem root. | |
| 24 | + | |
| 25 | + Returns a fully-expanded absolute path (no literal `~`) so subprocess | |
| 26 | + callers can pass it through to code that doesn't re-expand tildes. | |
| 27 | + | |
| 28 | + Called per-invocation rather than cached at import time so test | |
| 29 | + monkeypatches of the env vars take effect — the plugin's hooks each | |
| 30 | + run as fresh subprocesses in production, so the per-call cost is | |
| 31 | + negligible compared to subprocess spawn. | |
| 32 | + """ | |
| 33 | + explicit = os.environ.get("SECURITY_WARNINGS_STATE_DIR") | |
| 34 | + if explicit: | |
| 35 | + return os.path.expanduser(explicit) | |
| 36 | + cc_config = os.environ.get("CLAUDE_CONFIG_DIR") | |
| 37 | + if cc_config: | |
| 38 | + return os.path.expanduser(os.path.join(cc_config, "security")) | |
| 39 | + return os.path.expanduser("~/.claude/security") | |
| 40 | + | |
| 41 | + | |
| 13 | 42 | # Debug log file. Lives under the plugin state dir (default ~/.claude/security/) |
| 14 | 43 | # rather than /tmp because /tmp is world-writable on multi-user hosts (TOCTOU / |
| 15 | 44 | # symlink-attack surface, cross-user log leakage). Overridable per-process via |
| 16 | -# SECURITY_GUIDANCE_DEBUG_LOG, or per-state-dir via SECURITY_WARNINGS_STATE_DIR. | |
| 17 | -_DEFAULT_STATE_DIR = os.path.expanduser( | |
| 18 | - os.environ.get("SECURITY_WARNINGS_STATE_DIR") or "~/.claude/security" | |
| 19 | -) | |
| 45 | +# SECURITY_GUIDANCE_DEBUG_LOG, or per-state-dir via SECURITY_WARNINGS_STATE_DIR | |
| 46 | +# (plugin-specific override) or CLAUDE_CONFIG_DIR (CC-wide config dir, #1868). | |
| 20 | 47 | DEBUG_LOG_FILE = os.environ.get("SECURITY_GUIDANCE_DEBUG_LOG") or os.path.join( |
| 21 | - _DEFAULT_STATE_DIR, "log.txt" | |
| 48 | + state_dir(), "log.txt" | |
| 22 | 49 | ) |
| 23 | 50 | # Cap the debug log so parallel-worker fleets don't fill disk. When the active |
| 24 | 51 | # file exceeds this it's atomically rotated to <file>.1 (overwriting any prior |
| @@ -89,7 +116,18 @@ | ||
| 89 | 116 | # Emitted via _usage_metrics() into the existing emit_metrics() channel so |
| 90 | 117 | # hook metrics rows carry per-invocation token/cost totals |
| 91 | 118 | # alongside the existing skip_reason / vulns_found fields. |
| 92 | -_USAGE = {"in": 0, "out": 0, "cr": 0, "cw": 0, "cost": 0.0, "n": 0} | |
| 119 | +_USAGE = { | |
| 120 | + "in": 0, "out": 0, "cr": 0, "cw": 0, "cost": 0.0, "n": 0, | |
| 121 | + # HTTP error visibility (#2098 visibility gap — see emit comment in | |
| 122 | + # _usage_metrics). Without this, API failures from `_call_claude` left | |
| 123 | + # zero fingerprint in telemetry: the call returns None, the caller's | |
| 124 | + # emit_metrics carries no api_calls field, and the failure is | |
| 125 | + # indistinguishable from "no review needed". The deprecation outage | |
| 126 | + # that broke every commit-review LLM call was invisible until users | |
| 127 | + # reported it manually. | |
| 128 | + "http_err_last": 0, # most recent HTTP error code this invocation | |
| 129 | + "http_err_count": 0, # total HTTP errors (4xx + 5xx + network) | |
| 130 | +} | |
| 93 | 131 | _USAGE_LOCK = threading.Lock() |
| 94 | 132 | |
| 95 | 133 | # $/Mtok (input, output). Used only for the raw-HTTP path; the SDK path |
| @@ -102,6 +140,9 @@ | ||
| 102 | 140 | "claude-sonnet-4-6": (3.0, 15.0), |
| 103 | 141 | "claude-opus-4-6": (15.0, 75.0), |
| 104 | 142 | "claude-opus-4-7": (5.0, 25.0), |
| 143 | + "claude-opus-4-8": (5.0, 25.0), | |
| 144 | + "claude-opus-5": (5.0, 25.0), | |
| 145 | + "claude-opus-5-5": (4.0, 20.0), | |
| 105 | 146 | } |
| 106 | 147 | _PRICE_DEFAULT = (3.0, 15.0) |
| 107 | 148 | |
| @@ -139,19 +180,55 @@ | ||
| 139 | 180 | _USAGE["n"] += 1 |
| 140 | 181 | |
| 141 | 182 | |
| 183 | +def _record_http_error(status): | |
| 184 | + """Record an HTTP error from an LLM API call. `status` is the HTTP | |
| 185 | + status code (integer 400–599) or -1 for network/timeout errors. Stored | |
| 186 | + in `_USAGE["http_err_last"]` (most recent) and counted in | |
| 187 | + `_USAGE["http_err_count"]`. Snapshot via `_usage_metrics()` so every | |
| 188 | + subsequent `emit_metrics` includes the failure fingerprint. | |
| 189 | + | |
| 190 | + Background: without this, the most recent example was the #2098 | |
| 191 | + deprecation 400. Every hook fire's LLM call returned HTTP 400; the | |
| 192 | + plugin caught it and returned None; the emit_metrics carried no | |
| 193 | + api_calls field; aggregate dashboards looked normal. The failure | |
| 194 | + only became visible when a user manually reported errors out of | |
| 195 | + their debug log. With this field, a category-of-failure spike (4xx, | |
| 196 | + 5xx, or -1 network) is queryable from BQ in real time. | |
| 197 | + """ | |
| 198 | + try: | |
| 199 | + s = int(status) | |
| 200 | + except (TypeError, ValueError): | |
| 201 | + return | |
| 202 | + with _USAGE_LOCK: | |
| 203 | + _USAGE["http_err_last"] = s | |
| 204 | + _USAGE["http_err_count"] += 1 | |
| 205 | + | |
| 206 | + | |
| 142 | 207 | def _usage_metrics(): |
| 143 | 208 | """Snapshot the accumulator as metric keys. Returns {} when no API calls |
| 144 | - were made so skip-path emits don't burn key budget. cost_usd rounded to | |
| 145 | - 1e-6 to keep the float finite/short for the zod schema.""" | |
| 209 | + AND no HTTP errors were made so skip-path emits don't burn key budget. | |
| 210 | + cost_usd rounded to 1e-6 to keep the float finite/short for the zod | |
| 211 | + schema. | |
| 212 | + | |
| 213 | + HTTP errors (`http_err_last`, `http_err_count`) emitted ONLY when | |
| 214 | + `http_err_count > 0` so successful calls don't pad every metrics row | |
| 215 | + with two zero fields. | |
| 216 | + """ | |
| 146 | 217 | with _USAGE_LOCK: |
| 147 | - if _USAGE["n"] == 0: | |
| 218 | + if _USAGE["n"] == 0 and _USAGE["http_err_count"] == 0: | |
| 148 | 219 | return {} |
| 149 | - return { | |
| 150 | - "tok_in": _USAGE["in"], | |
| 151 | - "tok_out": _USAGE["out"], | |
| 152 | - "tok_cache_r": _USAGE["cr"], | |
| 153 | - "tok_cache_w": _USAGE["cw"], | |
| 154 | - "cost_usd": round(_USAGE["cost"], 6), | |
| 155 | - "api_calls": _USAGE["n"], | |
| 156 | - } | |
| 220 | + out = {} | |
| 221 | + if _USAGE["n"] > 0: | |
| 222 | + out.update({ | |
| 223 | + "tok_in": _USAGE["in"], | |
| 224 | + "tok_out": _USAGE["out"], | |
| 225 | + "tok_cache_r": _USAGE["cr"], | |
| 226 | + "tok_cache_w": _USAGE["cw"], | |
| 227 | + "cost_usd": round(_USAGE["cost"], 6), | |
| 228 | + "api_calls": _USAGE["n"], | |
| 229 | + }) | |
| 230 | + if _USAGE["http_err_count"] > 0: | |
| 231 | + out["http_err_last"] = _USAGE["http_err_last"] | |
| 232 | + out["http_err_count"] = _USAGE["http_err_count"] | |
| 233 | + return out | |
| 157 | 234 |
| @@ -13,7 +13,7 @@ | ||
| 13 | 13 | from gitutil import ( |
| 14 | 14 | GIT_CMD, |
| 15 | 15 | _git_dir, _git_toplevel, _git_status_porcelain, |
| 16 | - _git_rev_parse_head, _is_ancestor, _git_name_only, | |
| 16 | + _git_rev_parse_head, _is_ancestor, _git_name_only, _temp_index, | |
| 17 | 17 | ) |
| 18 | 18 | from session_state import with_locked_state |
| 19 | 19 | |
| @@ -71,7 +71,7 @@ | ||
| 71 | 71 | with_locked_state(session_id, _record) |
| 72 | 72 | |
| 73 | 73 | |
| 74 | -def consume_stop_state(session_id): | |
| 74 | +def consume_stop_state(session_id, clear=True): | |
| 75 | 75 | """Atomically snapshot all state the Stop hook needs and clear touched_paths. |
| 76 | 76 | |
| 77 | 77 | The Stop hook is asyncRewake — it runs in the background after Claude's |
| @@ -102,17 +102,27 @@ | ||
| 102 | 102 | "fire_count": 0 if expired else state.get("stop_hook_fire_count", 0), |
| 103 | 103 | "fire_count_expired": expired and state.get("stop_hook_fire_count", 0) > 0, |
| 104 | 104 | "previous_findings": [] if findings_expired else list(state.get("previous_findings", [])), |
| 105 | + "reviewed_diff_hash": state.get("reviewed_diff_hash"), | |
| 105 | 106 | } |
| 106 | - state["touched_paths"] = [] | |
| 107 | + if clear: | |
| 108 | + state["touched_paths"] = [] | |
| 109 | + state.pop("reviewed_diff_hash", None) | |
| 107 | 110 | return snap |
| 108 | 111 | |
| 109 | 112 | return with_locked_state(session_id, _snap) or { |
| 110 | 113 | "touched_paths": [], "baseline_sha": None, "head_at_capture": None, |
| 111 | 114 | "untracked_at_baseline": {}, |
| 112 | 115 | "fire_count": 0, "fire_count_expired": False, "previous_findings": [], |
| 116 | + "reviewed_diff_hash": None, | |
| 113 | 117 | } |
| 114 | 118 | |
| 115 | 119 | |
| 120 | +def record_reviewed_diff(session_id, diff_hash): | |
| 121 | + def _save(state): | |
| 122 | + state["reviewed_diff_hash"] = diff_hash | |
| 123 | + with_locked_state(session_id, _save) | |
| 124 | + | |
| 125 | + | |
| 116 | 126 | def restore_unreviewed_stop_state(session_id, paths, baseline_sha): |
| 117 | 127 | """Put consumed touched_paths back so the next Stop reviews them. |
| 118 | 128 | |
| @@ -138,7 +148,17 @@ | ||
| 138 | 148 | |
| 139 | 149 | |
| 140 | 150 | def get_baseline_file_content(session_id, file_path, cwd): |
| 141 | - """Get the content of a file at the baseline SHA. Returns None if unavailable.""" | |
| 151 | + """Get the content of a file at the baseline SHA. Returns None if unavailable. | |
| 152 | + | |
| 153 | + Decode the file content as UTF-8 with errors="replace" rather than using | |
| 154 | + text=True: source files in user repos can be latin-1 / cp1252 / shift-jis | |
| 155 | + / etc., and on Windows text=True would decode via locale.getpreferredencoding() | |
| 156 | + in strict mode and raise UnicodeDecodeError in the subprocess reader | |
| 157 | + thread — leaving result.stdout=None and propagating AttributeError when | |
| 158 | + the caller tries to use it. Same class as the existing migrations at | |
| 159 | + security_reminder_hook.py:540 (reflog subjects) and :1115 (commit | |
| 160 | + diffs); this helper was missed in that pass. See | |
| 161 | + anthropics/claude-plugins-official#2056.""" | |
| 142 | 162 | baseline_sha = load_baseline_sha(session_id) |
| 143 | 163 | if not baseline_sha: |
| 144 | 164 | return None |
| @@ -151,12 +171,12 @@ | ||
| 151 | 171 | return None |
| 152 | 172 | result = subprocess.run( |
| 153 | 173 | [*GIT_CMD, "show", f"{baseline_sha}:{rel_path}"], |
| 154 | - cwd=cwd, capture_output=True, text=True, timeout=5 | |
| 174 | + cwd=cwd, capture_output=True, timeout=5 | |
| 155 | 175 | ) |
| 156 | 176 | if result.returncode == 0: |
| 157 | - return result.stdout | |
| 177 | + return (result.stdout or b"").decode("utf-8", errors="replace") | |
| 158 | 178 | return None |
| 159 | - except (subprocess.TimeoutExpired, FileNotFoundError, OSError): | |
| 179 | + except (subprocess.TimeoutExpired, FileNotFoundError, OSError, ValueError): | |
| 160 | 180 | return None |
| 161 | 181 | |
| 162 | 182 | |
| @@ -168,38 +188,55 @@ | ||
| 168 | 188 | Falls back to HEAD if the working tree is clean. |
| 169 | 189 | Returns the SHA string, or None if not in a git repo or if the repo has no commits. |
| 170 | 190 | |
| 191 | + `stash create` runs on a throwaway copy of the index (GIT_INDEX_FILE), so | |
| 192 | + its `index.lock` and `index.stash.<pid>` files land next to the copy, not | |
| 193 | + in `.git`: it cannot collide with a concurrent git command, and a timeout | |
| 194 | + kill cannot leave a lock that blocks the user's next one. | |
| 195 | + GIT_OPTIONAL_LOCKS does not cover stash. Copying the real index without a | |
| 196 | + lock is safe: git replaces it by atomic rename. | |
| 197 | + | |
| 171 | 198 | NOTE: `git stash create` does NOT capture untracked files. UPS pairs this |
| 172 | 199 | SHA with a `_list_untracked()` snapshot stored as `untracked_at_baseline`, |
| 173 | 200 | and `compute_v2_review_set` subtracts that set so pre-existing untracked |
| 174 | 201 | files are not reviewed as Claude-authored. |
| 175 | 202 | """ |
| 203 | + # stdout is a SHA so text=True is safe on stdout, but a non-ASCII | |
| 204 | + # filename in `git stash create`'s STDERR warning (e.g. a worktree | |
| 205 | + # with `Ávila_report.txt` triggers a quotePath/locale warning) would | |
| 206 | + # trip the stderr reader thread on Windows cp1252. Decode both streams | |
| 207 | + # leniently for symmetry with _list_untracked. See #2056. | |
| 176 | 208 | try: |
| 177 | 209 | # Check if HEAD exists (i.e., repo has at least one commit) |
| 178 | 210 | head_check = subprocess.run( |
| 179 | 211 | [*GIT_CMD, "rev-parse", "HEAD"], |
| 180 | - cwd=cwd, capture_output=True, text=True, timeout=5 | |
| 212 | + cwd=cwd, capture_output=True, timeout=5 | |
| 181 | 213 | ) |
| 182 | 214 | if head_check.returncode != 0: |
| 183 | 215 | # No commits yet — skip review rather than creating commits in the user's repo |
| 184 | 216 | debug_log("No commits in repo, skipping baseline capture") |
| 185 | 217 | return None |
| 186 | 218 | |
| 187 | - result = subprocess.run( | |
| 188 | - [*GIT_CMD, "stash", "create"], | |
| 189 | - cwd=cwd, capture_output=True, text=True, timeout=15 | |
| 190 | - ) | |
| 191 | - sha = result.stdout.strip() | |
| 219 | + sha = "" | |
| 220 | + # untracked_paths=[]: a plain copy of the index, no `add -N` pass. | |
| 221 | + with _temp_index(cwd, untracked_paths=[]) as env: | |
| 222 | + # env is None when there is no index file to copy: treat as clean. | |
| 223 | + if env is not None: | |
| 224 | + result = subprocess.run( | |
| 225 | + [*GIT_CMD, "stash", "create"], | |
| 226 | + cwd=cwd, capture_output=True, timeout=15, env=env | |
| 227 | + ) | |
| 228 | + sha = (result.stdout or b"").decode("utf-8", errors="replace").strip() | |
| 192 | 229 | if sha: |
| 193 | 230 | return sha |
| 194 | 231 | |
| 195 | 232 | # Working tree is clean — stash create returns empty. Use HEAD. |
| 196 | 233 | result = subprocess.run( |
| 197 | 234 | [*GIT_CMD, "rev-parse", "HEAD"], |
| 198 | - cwd=cwd, capture_output=True, text=True, timeout=5 | |
| 235 | + cwd=cwd, capture_output=True, timeout=5 | |
| 199 | 236 | ) |
| 200 | - sha = result.stdout.strip() | |
| 237 | + sha = (result.stdout or b"").decode("utf-8", errors="replace").strip() | |
| 201 | 238 | return sha if sha else None |
| 202 | - except (subprocess.TimeoutExpired, FileNotFoundError, OSError) as e: | |
| 239 | + except (subprocess.TimeoutExpired, FileNotFoundError, OSError, ValueError) as e: | |
| 203 | 240 | debug_log(f"Failed to capture git baseline: {e}") |
| 204 | 241 | return None |
| 205 | 242 | |
| @@ -323,19 +360,35 @@ | ||
| 323 | 360 | mtime is captured so an in-place edit during the turn is still reviewed. |
| 324 | 361 | |
| 325 | 362 | Uses ls-files (not status) for the UPS path: the index diff isn't needed, |
| 326 | - and ls-files --others only walks the worktree against .gitignore.""" | |
| 363 | + and ls-files --others only walks the worktree against .gitignore. | |
| 364 | + | |
| 365 | + Decodes stdout/stderr as UTF-8 with errors="replace" instead of using | |
| 366 | + text=True. With core.quotePath=false git emits raw UTF-8 bytes for | |
| 367 | + non-ASCII filenames; text=True decodes via locale.getpreferredencoding() | |
| 368 | + in strict mode — on Windows that's cp1252 with several undefined bytes | |
| 369 | + (0x81/0x8D/0x8F/0x90/0x9D), all of which appear in UTF-8 encodings of | |
| 370 | + common accented capitals (Á Í Ï Ð Ý) and most CJK/emoji codepoints. | |
| 371 | + A non-ASCII filename in the worktree crashed the subprocess reader | |
| 372 | + thread, left r.stdout=None, and propagated AttributeError out of the | |
| 373 | + helper — silently losing the baseline snapshot every UserPromptSubmit. | |
| 374 | + See anthropics/claude-plugins-official#2056. The sibling helpers in | |
| 375 | + gitutil.py already follow the lenient pattern; this function and | |
| 376 | + capture_git_baseline / _git_name_only / _git_status_porcelain were | |
| 377 | + the holdouts.""" | |
| 327 | 378 | try: |
| 328 | 379 | repo = _git_toplevel(cwd) or cwd |
| 380 | + # core.quotePath=false comes from GIT_CMD globally (see gitutil.py). | |
| 329 | 381 | r = subprocess.run( |
| 330 | - [*GIT_CMD, "-c", "core.quotePath=false", "ls-files", | |
| 331 | - "--others", "--exclude-standard", "-z"], | |
| 332 | - cwd=repo, capture_output=True, text=True, timeout=15, | |
| 382 | + [*GIT_CMD, "ls-files", "--others", "--exclude-standard", "-z"], | |
| 383 | + cwd=repo, capture_output=True, timeout=15, | |
| 333 | 384 | ) |
| 334 | 385 | if r.returncode != 0: |
| 335 | - debug_log(f"_list_untracked rc={r.returncode}: {r.stderr[:200]}") | |
| 386 | + stderr_str = (r.stderr or b"").decode("utf-8", errors="replace") | |
| 387 | + debug_log(f"_list_untracked rc={r.returncode}: {stderr_str[:200]}") | |
| 336 | 388 | return {} |
| 389 | + stdout = (r.stdout or b"").decode("utf-8", errors="replace") | |
| 337 | 390 | out = {} |
| 338 | - for p in r.stdout.split("\0"): | |
| 391 | + for p in stdout.split("\0"): | |
| 339 | 392 | if not p: |
| 340 | 393 | continue |
| 341 | 394 | try: |
| @@ -346,7 +399,9 @@ | ||
| 346 | 399 | debug_log(f"_list_untracked: capped at {UNTRACKED_BASELINE_CAP}") |
| 347 | 400 | break |
| 348 | 401 | return out |
| 349 | - except (subprocess.TimeoutExpired, FileNotFoundError, OSError) as e: | |
| 402 | + except (subprocess.TimeoutExpired, FileNotFoundError, OSError, ValueError) as e: | |
| 403 | + # ValueError guards against any future strict-decode regression | |
| 404 | + # so the helper degrades to {} instead of crashing the hook. | |
| 350 | 405 | debug_log(f"_list_untracked error: {e}") |
| 351 | 406 | return {} |
| 352 | 407 | |
| @@ -407,7 +462,7 @@ | ||
| 407 | 462 | dirty_now |= _git_name_only(repo, f"{head_at_capture}..HEAD") or set() |
| 408 | 463 | diff_base = head_at_capture |
| 409 | 464 | |
| 410 | - # changed_since: tracked files vs the stash baseline (no temp index — the | |
| 465 | + # changed_since: tracked files vs the stash baseline (no `add -N` — the | |
| 411 | 466 | # stash never contained untracked files anyway), then union with |
| 412 | 467 | # currently-untracked. The previous `include_untracked=True` arm cost a |
| 413 | 468 | # full `git add -N .` (slow in large repos) per call to surface |
| @@ -23,15 +23,436 @@ | ||
| 23 | 23 | import time |
| 24 | 24 | from pathlib import Path |
| 25 | 25 | |
| 26 | +# Shared state-dir resolver: SECURITY_WARNINGS_STATE_DIR → CLAUDE_CONFIG_DIR/security | |
| 27 | +# → ~/.claude/security. See _base.state_dir for resolution precedence. Re-aliased | |
| 28 | +# here to match the existing local name (state_dir was already a local var in | |
| 29 | +# main() and _maybe_emit_user_notice). | |
| 30 | +from _base import state_dir as _resolve_state_dir | |
| 31 | + | |
| 26 | 32 | # Outcome codes for the sdk_bootstrap metric. Values are stable for telemetry. |
| 27 | 33 | NOOP_SYSTEM = 0 # claude_agent_sdk already importable in system python |
| 28 | 34 | NOOP_VENV = 1 # venv already built and SDK imports from it |
| 29 | 35 | BUILT = 2 # venv created + SDK pip-installed this run |
| 30 | 36 | BUILD_FAILED = 3 # venv create or pip install raised/timed out |
| 31 | -SKIP_WIN32 = 4 # Windows; consumer glob doesn't handle Lib/ layout | |
| 37 | +# Outcome 4 was previously SKIP_WIN32; retired now that the consumer glob in | |
| 38 | +# llm.py also matches Windows venv layout (Lib/site-packages). Don't reuse the | |
| 39 | +# value — telemetry rows from older plugin builds still emit 4. | |
| 32 | 40 | SKIP_SENTINEL = 5 # another SessionStart is currently building |
| 41 | +HOOK_PY_INCOMPATIBLE = 6 # hook interpreter is <3.10 — SDK syntax can't load | |
| 42 | + # here no matter how the venv was built. See #2071. | |
| 43 | +# --target fallback: when `python -m venv` can't bootstrap pip (ensurepip | |
| 44 | +# missing — Debian python3-venv not installed, or a python.org/pyenv build | |
| 45 | +# without ensurepip), fall back to `pip install --target <dir>` which needs | |
| 46 | +# only the system pip, not venv/ensurepip. Telemetry (v2.0.4 sdk_has_pip | |
| 47 | +# probe) confirmed ~95% of venv_ensurepip_fail users HAVE pip, so this | |
| 48 | +# recovers the agentic reviewer for them instead of degrading to pattern + | |
| 49 | +# single-shot review. See #2154 follow-up. | |
| 50 | +BUILT_TARGET = 7 # venv ensurepip failed → SDK pip-installed via --target | |
| 51 | +NOOP_TARGET = 8 # --target libs already present and importable | |
| 52 | +SKIP_COOLDOWN = 9 # a recent build was signal-killed (memory pressure) — not | |
| 53 | + # retrying this session to avoid burning the user's | |
| 54 | + # memory/CPU on a build that keeps getting killed. CCR | |
| 55 | + # repro confirmed the dominant Linux BUILD_FAILED is a | |
| 56 | + # SIGKILL/SIGSEGV of the memory-heavy venv+pip subprocess | |
| 57 | + # (rc<0, empty streams). See #2154 follow-up. | |
| 33 | 58 | |
| 59 | +# How long to skip rebuilds after a signal kill. Retries at most once per | |
| 60 | +# window so a machine whose memory frees up still recovers (just not every | |
| 61 | +# session). Keyed by marker mtime. | |
| 62 | +SIGNAL_KILL_COOLDOWN_SEC = 24 * 3600 | |
| 34 | 63 | |
| 64 | + | |
| 65 | +# Phase + err-kind integer encoding for sdk_bootstrap_phase / sdk_bootstrap_err. | |
| 66 | +# | |
| 67 | +# Earlier versions emitted these as STRINGS (e.g. "pip", "dns_fail"). CC's | |
| 68 | +# plugin-metrics pipeline silently drops plugin-emitted string values — | |
| 69 | +# only `bool|finite-number` plugin metrics reach BigQuery. (CC-core | |
| 70 | +# metrics like `subscription_type` are exempt because they're injected | |
| 71 | +# downstream of plugin validation.) Confirmed empirically: 185K | |
| 72 | +# BUILD_FAILED rows in BQ had `sdk_bootstrap_phase`/`sdk_bootstrap_err` | |
| 73 | +# = NULL despite the Python code emitting them. This left ~28K | |
| 74 | +# BUILD_FAILED sessions/day with no diagnostic split — flying blind on | |
| 75 | +# the real failure modes (pip-no-match vs dns-fail vs ssl-verify etc.). | |
| 76 | +# | |
| 77 | +# Fix: encode as small integers per the maps below. Values are | |
| 78 | +# APPEND-ONLY for telemetry stability. Reserve 99 as the "unknown / | |
| 79 | +# uncategorized" bucket so an unmapped err_kind (e.g., a new exception | |
| 80 | +# type) still emits a non-zero signal. | |
| 81 | +SDK_BOOTSTRAP_PHASE_CODES = { | |
| 82 | + "pre": 1, # pre-venv (state_dir.mkdir, sentinel open) | |
| 83 | + "venv": 2, # python -m venv --clear | |
| 84 | + "pip": 3, # pip install | |
| 85 | + "main": 4, # uncaught exception above main() | |
| 86 | + "pip_target": 5, # `pip install --target` fallback (venv ensurepip failed) | |
| 87 | +} | |
| 88 | +SDK_BOOTSTRAP_ERR_CODES = { | |
| 89 | + "pip_no_match": 1, | |
| 90 | + "dns_fail": 2, | |
| 91 | + "conn_refused": 3, | |
| 92 | + "ssl_verify": 4, | |
| 93 | + "perm_denied": 5, | |
| 94 | + "no_pip": 6, | |
| 95 | + "disk_full": 7, | |
| 96 | + "proxy_auth": 8, | |
| 97 | + "stderr_timeout": 9, # pip stderr containing "timeout"/"timed out" | |
| 98 | + "subprocess_timeout": 10, # subprocess.TimeoutExpired (>120s) | |
| 99 | + "signal_killed": 16, # venv/pip subprocess killed by a signal | |
| 100 | + # (rc<0 or 128+sig) — OOM-killer SIGKILL / | |
| 101 | + # RLIMIT_AS SIGSEGV, empty streams. The | |
| 102 | + # actual rc rides in sdk_bootstrap_rc. This | |
| 103 | + # is the dominant Linux failure (CCR repro). | |
| 104 | + # Venv-stage specific categories added after PR #2112 telemetry surfaced | |
| 105 | + # 2,406 phase=2/err=99 sessions in the first 3h of v2.0.1 — venv phase | |
| 106 | + # failing in ways the original pip-flavored patterns didn't catch. These | |
| 107 | + # all split out of what was previously collapsing to _uncategorized. | |
| 108 | + "venv_ensurepip_fail": 11, # Debian/Ubuntu missing python3-venv; | |
| 109 | + # stderr mentions ensurepip non-zero exit | |
| 110 | + # or "ensurepip is not available" | |
| 111 | + "venv_path_too_long": 12, # Windows MAX_PATH (260) or POSIX | |
| 112 | + # ENAMETOOLONG — venv writes deep paths | |
| 113 | + # under state_dir/agent-sdk-venv/Lib/... | |
| 114 | + "venv_no_module": 13, # `python3 -m venv` itself missing — "No | |
| 115 | + # module named 'venv'" / "No module named venv" | |
| 116 | + "venv_already_exists": 14, # Errno 17 / "file exists" — sentinel race | |
| 117 | + # past O_EXCL or stale dir survived --clear | |
| 118 | + "venv_setup_failed": 15, # Generic "virtual environment was not | |
| 119 | + # created successfully" — catches the long | |
| 120 | + # tail of venv setup failures that don't | |
| 121 | + # match a more specific category above | |
| 122 | + # 16–98 reserved for future categories; APPEND-ONLY. | |
| 123 | + # 99 catches everything else (including "exc:<TypeName>" and "other:<tail>" | |
| 124 | + # — the original string is debug-loggable but the integer is what makes | |
| 125 | + # it to telemetry). For the "other:" tail, `sdk_bootstrap_stderr_sig` | |
| 126 | + # carries a bounded integer hash so we can still distinguish patterns | |
| 127 | + # in BQ aggregation. | |
| 128 | + "_uncategorized": 99, | |
| 129 | +} | |
| 130 | + | |
| 131 | +# Exception-type encoding for the "exc:<TypeName>" err_kinds (the generic | |
| 132 | +# `except Exception` path — venv/pip raised a Python exception rather than | |
| 133 | +# a CalledProcessError with categorizable stderr). | |
| 134 | +# | |
| 135 | +# #2154 telemetry surfaced that the dominant remaining venv BUILD_FAILED | |
| 136 | +# bucket (phase=venv, err=99) is ~99% `exc:` with stderr_sig=NULL — i.e. | |
| 137 | +# exceptions, not stderr-bearing subprocess failures — so the stderr_sig | |
| 138 | +# hash couldn't distinguish them. This maps the exception TYPE to a stable | |
| 139 | +# code so BQ can tell FileNotFoundError (python/venv binary missing) from | |
| 140 | +# PermissionError (read-only home) from a bare OSError, etc. | |
| 141 | +# | |
| 142 | +# All the FileNotFoundError/PermissionError/etc. entries are OSError | |
| 143 | +# subclasses, so they ALSO carry an errno (see _encode_errno) — the type | |
| 144 | +# code gives the Python class, errno gives the OS-level cause. APPEND-ONLY. | |
| 145 | +SDK_BOOTSTRAP_EXC_CODES = { | |
| 146 | + "FileNotFoundError": 1, # interpreter/venv path component missing | |
| 147 | + "PermissionError": 2, # read-only home, sandboxed FS | |
| 148 | + "NotADirectoryError": 3, | |
| 149 | + "IsADirectoryError": 4, | |
| 150 | + "FileExistsError": 5, # (sentinel race is handled separately; this | |
| 151 | + # is FileExistsError from elsewhere in venv) | |
| 152 | + "OSError": 6, # bare OSError — errno carries the real cause | |
| 153 | + "BlockingIOError": 7, | |
| 154 | + "BrokenPipeError": 8, | |
| 155 | + "ConnectionError": 9, | |
| 156 | + "TimeoutError": 10, # distinct from subprocess.TimeoutExpired | |
| 157 | + "InterruptedError": 11, | |
| 158 | + "MemoryError": 12, | |
| 159 | + "UnicodeDecodeError": 13, | |
| 160 | + "ValueError": 14, | |
| 161 | + "RuntimeError": 15, | |
| 162 | + # 16–98 reserved; APPEND-ONLY. | |
| 163 | + "_other_exc": 99, # an exception type not in this map | |
| 164 | +} | |
| 165 | + | |
| 166 | + | |
| 167 | +def _encode_phase(s): | |
| 168 | + """Map err_phase string to its telemetry integer code, or 0 if unset. | |
| 169 | + Empty/None → 0 lets `if encoded:` cleanly skip emission. Per | |
| 170 | + SDK_BOOTSTRAP_PHASE_CODES, valid codes are 1-4.""" | |
| 171 | + return SDK_BOOTSTRAP_PHASE_CODES.get((s or "").strip(), 0) | |
| 172 | + | |
| 173 | + | |
| 174 | +def _encode_err_kind(s): | |
| 175 | + """Map err_kind string to its telemetry integer code, or 0 if unset. | |
| 176 | + Direct hits use the static map; "exc:<X>" and "other:<tail>" both | |
| 177 | + collapse to _uncategorized (99) — the raw string survives in debug | |
| 178 | + logs, only the integer reaches BQ.""" | |
| 179 | + s = (s or "").strip() | |
| 180 | + if not s: | |
| 181 | + return 0 | |
| 182 | + if s in SDK_BOOTSTRAP_ERR_CODES: | |
| 183 | + return SDK_BOOTSTRAP_ERR_CODES[s] | |
| 184 | + # "signal_killed:<rc>" carries the returncode in sdk_bootstrap_rc; the | |
| 185 | + # category maps to the signal_killed code. | |
| 186 | + if s.startswith("signal_killed"): | |
| 187 | + return SDK_BOOTSTRAP_ERR_CODES["signal_killed"] | |
| 188 | + # Prefix matches for the catch-all categories | |
| 189 | + if s.startswith("exc:") or s.startswith("other:") or s == "other": | |
| 190 | + return SDK_BOOTSTRAP_ERR_CODES["_uncategorized"] | |
| 191 | + # Unknown string — still emit as uncategorized rather than dropping | |
| 192 | + return SDK_BOOTSTRAP_ERR_CODES["_uncategorized"] | |
| 193 | + | |
| 194 | + | |
| 195 | +def _encode_rc(err_kind): | |
| 196 | + """Extract the subprocess returncode embedded in a 'signal_killed:<rc>' | |
| 197 | + err_kind (e.g. -11 SIGSEGV / -9 SIGKILL / 139 shell-wrapped). Emitted as | |
| 198 | + sdk_bootstrap_rc so BQ can tell OOM-killer (-9) from RLIMIT_AS (-11). | |
| 199 | + Returns 0 when absent/non-numeric.""" | |
| 200 | + if not err_kind or not err_kind.startswith("signal_killed:"): | |
| 201 | + return 0 | |
| 202 | + try: | |
| 203 | + return int(err_kind.split(":", 1)[1]) | |
| 204 | + except (ValueError, IndexError): | |
| 205 | + return 0 | |
| 206 | + | |
| 207 | + | |
| 208 | +def _is_signal_kill(returncode) -> bool: | |
| 209 | + """A subprocess killed by a signal rather than a clean non-zero exit. | |
| 210 | + subprocess.run (no shell, as used here) reports negative rc = -signum | |
| 211 | + (SIGKILL→-9 OOM-killer, SIGSEGV→-11 RLIMIT_AS, SIGABRT→-6). The 128+sig | |
| 212 | + forms (134/137/139) are defensive for any shell-wrapped path. Paired with | |
| 213 | + empty stdout+stderr this is the memory-kill signature (CCR repro).""" | |
| 214 | + if returncode is None: | |
| 215 | + return False | |
| 216 | + return returncode < 0 or returncode in (134, 137, 139) | |
| 217 | + | |
| 218 | + | |
| 219 | +def _cooldown_remaining(state_dir) -> float: | |
| 220 | + """Seconds left in the signal-kill cooldown (0 if none/expired). Reads the | |
| 221 | + marker's mtime; a missing/unreadable marker means not in cooldown.""" | |
| 222 | + marker = Path(state_dir) / "agent-sdk-venv.cooldown" | |
| 223 | + try: | |
| 224 | + age = time.time() - marker.stat().st_mtime | |
| 225 | + except OSError: | |
| 226 | + return 0.0 | |
| 227 | + return max(0.0, SIGNAL_KILL_COOLDOWN_SEC - age) | |
| 228 | + | |
| 229 | + | |
| 230 | +def _write_cooldown(state_dir) -> None: | |
| 231 | + """Start/refresh the signal-kill cooldown so we stop re-attempting a build | |
| 232 | + that keeps getting killed every session. Best-effort.""" | |
| 233 | + try: | |
| 234 | + Path(state_dir).mkdir(parents=True, exist_ok=True) | |
| 235 | + (Path(state_dir) / "agent-sdk-venv.cooldown").write_text( | |
| 236 | + time.strftime("%Y-%m-%dT%H:%M:%SZ", time.gmtime())) | |
| 237 | + except OSError: | |
| 238 | + pass | |
| 239 | + | |
| 240 | + | |
| 241 | +def _encode_stderr_sig(err_kind): | |
| 242 | + """Bounded integer hash of the stderr tail captured in "other:<tail>" | |
| 243 | + err_kinds. Lets us distinguish patterns INSIDE the _uncategorized | |
| 244 | + (code 99) bucket without unbounded cardinality. | |
| 245 | + | |
| 246 | + Returns 0 for non-"other:" err_kinds (so the field auto-omits from | |
| 247 | + emit_metrics on categorized failures — see the emit block in main()). | |
| 248 | + | |
| 249 | + Strategy: take the tail's first ~30 chars (post-lowercase, post-trim), | |
| 250 | + SHA-1, fold the first 2 bytes to 0–999. Different stderr messages | |
| 251 | + cluster into different buckets; same stderr always maps to the same | |
| 252 | + bucket. Cardinality is bounded at 1000, well below any "high | |
| 253 | + cardinality" alarm — and a real failure mode typically produces | |
| 254 | + near-identical stderr across thousands of machines, so 1000 buckets | |
| 255 | + is comfortably wide. | |
| 256 | + | |
| 257 | + Why first ~30 chars: stderr like "ERROR: Command failed: <full | |
| 258 | + path>" varies the tail wildly (paths) but the categorization signal | |
| 259 | + is in the leading words. Dropping the suffix focuses the hash on | |
| 260 | + the discriminative part. | |
| 261 | + """ | |
| 262 | + if not err_kind or not err_kind.startswith("other:"): | |
| 263 | + return 0 | |
| 264 | + import hashlib | |
| 265 | + tail = err_kind[len("other:"):].strip().lower()[:30] | |
| 266 | + if not tail: | |
| 267 | + return 0 | |
| 268 | + h = hashlib.sha1(tail.encode("utf-8", errors="replace")).digest() | |
| 269 | + return int.from_bytes(h[:2], "big") % 1000 | |
| 270 | + | |
| 271 | + | |
| 272 | +def _encode_exc_kind(err_kind): | |
| 273 | + """Map an "exc:<TypeName>[:errno]" err_kind to its exception-type code | |
| 274 | + (SDK_BOOTSTRAP_EXC_CODES). Returns 0 for non-exc err_kinds (so the | |
| 275 | + sdk_bootstrap_exc field auto-omits on stderr/categorized failures). | |
| 276 | + Unmapped exception types → 99 (_other_exc).""" | |
| 277 | + if not err_kind or not err_kind.startswith("exc:"): | |
| 278 | + return 0 | |
| 279 | + # "exc:OSError:28" → "OSError"; "exc:RuntimeError" → "RuntimeError" | |
| 280 | + name = err_kind[len("exc:"):].split(":", 1)[0].strip() | |
| 281 | + if not name: | |
| 282 | + return 0 | |
| 283 | + return SDK_BOOTSTRAP_EXC_CODES.get(name, SDK_BOOTSTRAP_EXC_CODES["_other_exc"]) | |
| 284 | + | |
| 285 | + | |
| 286 | +def _encode_errno(err_kind): | |
| 287 | + """Extract the OS errno from an "exc:<TypeName>:<errno>" err_kind. | |
| 288 | + OSError-family exceptions embed their errno (ENOENT=2, EACCES=13, | |
| 289 | + ENOSPC=28, …) — the OS-level cause is far more actionable than the | |
| 290 | + Python class alone. Returns 0 when absent/non-numeric (field omitted).""" | |
| 291 | + if not err_kind or not err_kind.startswith("exc:"): | |
| 292 | + return 0 | |
| 293 | + parts = err_kind.split(":") | |
| 294 | + if len(parts) < 3: | |
| 295 | + return 0 | |
| 296 | + try: | |
| 297 | + return int(parts[2]) | |
| 298 | + except (ValueError, IndexError): | |
| 299 | + return 0 | |
| 300 | + | |
| 301 | + | |
| 302 | +def _probe_has_pip() -> bool: | |
| 303 | + """True iff the current interpreter can run pip (`-m pip --version`). | |
| 304 | + | |
| 305 | + Probed only on the venv_ensurepip_fail path (see __main__), NOT on the | |
| 306 | + happy path — it's an extra subprocess we only want when diagnosing a | |
| 307 | + failure. The result decides whether a `pip install --target` fallback | |
| 308 | + (Option A) is even viable for this machine: ensurepip/venv missing but | |
| 309 | + pip present → --target would work; pip also missing → it wouldn't, and | |
| 310 | + the user needs a system package (python3-venv / a complete Python).""" | |
| 311 | + try: | |
| 312 | + r = subprocess.run( | |
| 313 | + [sys.executable, "-m", "pip", "--version"], | |
| 314 | + capture_output=True, timeout=10, | |
| 315 | + ) | |
| 316 | + return r.returncode == 0 | |
| 317 | + except Exception: | |
| 318 | + return False | |
| 319 | + | |
| 320 | + | |
| 321 | +def _probe_alt_python() -> int: | |
| 322 | + """When the hook interpreter is <3.10 (HOOK_PY_INCOMPATIBLE), look for a | |
| 323 | + 3.10+ interpreter at well-known install locations that aren't necessarily | |
| 324 | + on the hook's PATH — Homebrew (/opt/homebrew, /usr/local), python.org | |
| 325 | + framework builds, and the `py`/distro layouts. Returns the HIGHEST version | |
| 326 | + found encoded as major*100+minor (e.g. 312), or 0 if none. | |
| 327 | + | |
| 328 | + Purpose (telemetry only, for now): size how many of the macOS Python-3.9 | |
| 329 | + cohort actually HAVE a newer interpreter that sg-python.sh's PATH probe | |
| 330 | + missed — i.e. how many are RECOVERABLE by an explicit-path search vs. | |
| 331 | + genuinely 3.9-only. Emitted as sdk_alt_py. Existence-checks the versioned | |
| 332 | + binaries (cheap); a later explicit-path search would version-verify before | |
| 333 | + exec'ing. Probed only on the incompatible path, so healthy sessions never | |
| 334 | + pay for it.""" | |
| 335 | + candidates = [] | |
| 336 | + for minor in (14, 13, 12, 11, 10): | |
| 337 | + candidates += [ | |
| 338 | + f"/opt/homebrew/bin/python3.{minor}", # Apple-Silicon Homebrew | |
| 339 | + f"/usr/local/bin/python3.{minor}", # Intel Homebrew / python.org shim | |
| 340 | + f"/Library/Frameworks/Python.framework/Versions/3.{minor}/bin/python3", # python.org | |
| 341 | + f"/usr/bin/python3.{minor}", # distro-managed (Linux) | |
| 342 | + ] | |
| 343 | + best = 0 | |
| 344 | + for path in candidates: | |
| 345 | + try: | |
| 346 | + if os.access(path, os.X_OK): | |
| 347 | + # path name encodes the minor; parse it back to a code | |
| 348 | + base = os.path.basename(path) | |
| 349 | + minor = None | |
| 350 | + if base.startswith("python3."): | |
| 351 | + minor = int(base.split(".")[1]) | |
| 352 | + elif "/Versions/3." in path: | |
| 353 | + minor = int(path.split("/Versions/3.")[1].split("/")[0]) | |
| 354 | + if minor is not None: | |
| 355 | + best = max(best, 300 + minor) | |
| 356 | + except (OSError, ValueError, IndexError): | |
| 357 | + continue | |
| 358 | + return best | |
| 359 | + | |
| 360 | + | |
| 361 | +def _pip_err_from_stderr(stderr_b): | |
| 362 | + """Categorize a pip-install stderr into a known err_kind (the pip subset | |
| 363 | + of SDK_BOOTSTRAP_ERR_CODES). Used by the --target fallback; mirrors the | |
| 364 | + pip branches of main()'s inline categorizer. Kept as a sibling rather | |
| 365 | + than extracting main()'s chain (which also has venv-phase branches) to | |
| 366 | + avoid disturbing the working venv categorization.""" | |
| 367 | + if isinstance(stderr_b, bytes): | |
| 368 | + s = stderr_b.decode("utf-8", errors="replace") | |
| 369 | + else: | |
| 370 | + s = str(stderr_b or "") | |
| 371 | + low = s.lower() | |
| 372 | + if "no matching distribution" in low or "could not find a version" in low: | |
| 373 | + return "pip_no_match" | |
| 374 | + if ("name or service not known" in low or "name resolution" in low | |
| 375 | + or "nodename nor servname" in low or "temporary failure in name" in low): | |
| 376 | + return "dns_fail" | |
| 377 | + if "connection refused" in low or "connection reset" in low: | |
| 378 | + return "conn_refused" | |
| 379 | + if "ssl" in low and ("verify" in low or "certificate" in low): | |
| 380 | + return "ssl_verify" | |
| 381 | + if "permission denied" in low or "read-only file system" in low: | |
| 382 | + return "perm_denied" | |
| 383 | + if "no module named pip" in low or "no module named ensurepip" in low: | |
| 384 | + return "no_pip" | |
| 385 | + if "no space left" in low or "disk quota" in low: | |
| 386 | + return "disk_full" | |
| 387 | + if "proxy" in low and ("authent" in low or "tunnel" in low or "407" in low): | |
| 388 | + return "proxy_auth" | |
| 389 | + if "timeout" in low or "timed out" in low: | |
| 390 | + return "stderr_timeout" | |
| 391 | + tail = next((ln.strip() for ln in reversed(s.splitlines()) if ln.strip()), "")[:60] | |
| 392 | + return f"other:{tail}" if tail else "other" | |
| 393 | + | |
| 394 | + | |
| 395 | +def _target_dir(state_dir) -> Path: | |
| 396 | + return Path(state_dir) / "agent-sdk-libs" | |
| 397 | + | |
| 398 | + | |
| 399 | +def _target_sdk_importable(state_dir) -> bool: | |
| 400 | + """True iff the --target libs dir has an importable claude_agent_sdk, | |
| 401 | + probed with THIS interpreter (the one llm.py will import it from) and the | |
| 402 | + target dir prepended to sys.path. Cheap dir-check first to avoid a | |
| 403 | + subprocess on the common no-target path.""" | |
| 404 | + target = _target_dir(state_dir) | |
| 405 | + if not (target / "claude_agent_sdk").is_dir(): | |
| 406 | + return False | |
| 407 | + try: | |
| 408 | + r = subprocess.run( | |
| 409 | + [sys.executable, "-c", | |
| 410 | + "import sys; sys.path.insert(0, sys.argv[1]); import claude_agent_sdk", | |
| 411 | + str(target)], | |
| 412 | + capture_output=True, timeout=10, | |
| 413 | + ) | |
| 414 | + return r.returncode == 0 | |
| 415 | + except Exception: | |
| 416 | + return False | |
| 417 | + | |
| 418 | + | |
| 419 | +def _build_via_target(state_dir) -> tuple[int, str, str]: | |
| 420 | + """Fallback install when `python -m venv` can't bootstrap pip (ensurepip | |
| 421 | + missing — Debian python3-venv absent, or a python.org/pyenv build without | |
| 422 | + ensurepip). `pip install --target <dir>` needs only the system pip, not | |
| 423 | + venv/ensurepip. v2.0.4 telemetry (sdk_has_pip) confirmed ~95% of | |
| 424 | + venv_ensurepip_fail users have pip. The consumer (llm.py) adds this flat | |
| 425 | + dir to sys.path. Returns (outcome, err_phase, err_kind). | |
| 426 | + | |
| 427 | + --upgrade so a stale/partial target dir from a prior failed attempt | |
| 428 | + doesn't make pip refuse; --prefer-binary mirrors the venv path's wheel | |
| 429 | + preference (ARM64 Windows cryptography).""" | |
| 430 | + target = _target_dir(state_dir) | |
| 431 | + try: | |
| 432 | + subprocess.run( | |
| 433 | + [sys.executable, "-m", "pip", "install", | |
| 434 | + "--target", str(target), "--upgrade", | |
| 435 | + "--disable-pip-version-check", "--prefer-binary", "--no-cache-dir", | |
| 436 | + "claude-agent-sdk"], | |
| 437 | + capture_output=True, timeout=120, check=True, | |
| 438 | + ) | |
| 439 | + return BUILT_TARGET, "", "" | |
| 440 | + except subprocess.CalledProcessError as e: | |
| 441 | + # A --target pip install is also memory-heavy, so it too can be | |
| 442 | + # signal-killed under memory pressure — cool down, same as the venv path. | |
| 443 | + if _is_signal_kill(e.returncode): | |
| 444 | + _write_cooldown(state_dir) | |
| 445 | + return BUILD_FAILED, "pip_target", f"signal_killed:{e.returncode}" | |
| 446 | + return BUILD_FAILED, "pip_target", _pip_err_from_stderr(e.stderr) | |
| 447 | + except subprocess.TimeoutExpired: | |
| 448 | + return BUILD_FAILED, "pip_target", "subprocess_timeout" | |
| 449 | + except Exception as e: | |
| 450 | + errno = getattr(e, "errno", None) | |
| 451 | + if isinstance(errno, int): | |
| 452 | + return BUILD_FAILED, "pip_target", f"exc:{type(e).__name__}:{errno}" | |
| 453 | + return BUILD_FAILED, "pip_target", f"exc:{type(e).__name__}" | |
| 454 | + | |
| 455 | + | |
| 35 | 456 | def _sdk_on_syspath() -> bool: |
| 36 | 457 | # find_spec is ~10ms; actually importing the SDK pulls in |
| 37 | 458 | # transitive deps and costs ~800ms — too heavy for a |
| @@ -60,22 +481,39 @@ | ||
| 60 | 481 | err_phase / err_kind are non-empty only on BUILD_FAILED — they let |
| 61 | 482 | telemetry split bootstrap failures by root cause. |
| 62 | 483 | """ |
| 63 | - # Windows venv layout (Lib/site-packages, no python* subdir) isn't | |
| 64 | - # handled by the consumer's glob in security_reminder_hook.py; skip the | |
| 65 | - # bootstrap entirely rather than build a venv that's never read. | |
| 66 | - if sys.platform == "win32": | |
| 67 | - return SKIP_WIN32, "", "" | |
| 484 | + # Honesty check (fixes the misleading NOOP_VENV in #2071): the SDK | |
| 485 | + # requires Python >=3.10 and uses 3.10+ syntax (match statements, | |
| 486 | + # PEP 604 unions). On a 3.9 hook interpreter we CANNOT import it no | |
| 487 | + # matter how the venv was built — llm.py runs in this same interpreter | |
| 488 | + # and the syntax-level import will SyntaxError. macOS ships 3.9.6 as | |
| 489 | + # the default `python3` and `/usr/bin` precedes Homebrew in PATH, so | |
| 490 | + # this case is the default state for a large share of macOS users. | |
| 491 | + # | |
| 492 | + # sg-python.sh now prefers python3.10+ binaries so most users won't | |
| 493 | + # reach this branch; the fallback to 3.9 is preserved for the | |
| 494 | + # pattern-warning hooks that don't need the SDK. Reporting | |
| 495 | + # HOOK_PY_INCOMPATIBLE here: | |
| 496 | + # (a) avoids 30-60s of wasted pip install, | |
| 497 | + # (b) avoids the lie where the venv_py probe says NOOP_VENV but the | |
| 498 | + # consumer import fails, and | |
| 499 | + # (c) gives telemetry a clean bucket to size the affected fleet. | |
| 500 | + if sys.version_info < (3, 10): | |
| 501 | + return ( | |
| 502 | + HOOK_PY_INCOMPATIBLE, | |
| 503 | + "hook_py", | |
| 504 | + f"py_{sys.version_info[0]}.{sys.version_info[1]}", | |
| 505 | + ) | |
| 68 | 506 | |
| 69 | - | |
| 70 | 507 | if _sdk_on_syspath(): |
| 71 | 508 | return NOOP_SYSTEM, "", "" |
| 72 | 509 | |
| 73 | - state_dir = Path( | |
| 74 | - os.environ.get("SECURITY_WARNINGS_STATE_DIR") | |
| 75 | - or os.path.expanduser("~/.claude/security") | |
| 76 | - ) | |
| 510 | + state_dir = Path(_resolve_state_dir()) | |
| 77 | 511 | venv = state_dir / "agent-sdk-venv" |
| 78 | - venv_py = venv / "bin" / "python" | |
| 512 | + # Windows venvs put the interpreter at Scripts\python.exe; POSIX uses bin/python. | |
| 513 | + if sys.platform == "win32": | |
| 514 | + venv_py = venv / "Scripts" / "python.exe" | |
| 515 | + else: | |
| 516 | + venv_py = venv / "bin" / "python" | |
| 79 | 517 | |
| 80 | 518 | # Another SessionStart (concurrent CC instance, same plugin) may already |
| 81 | 519 | # be building. The sentinel lives NEXT TO the venv, not inside it — |
| @@ -103,6 +541,20 @@ | ||
| 103 | 541 | except Exception: |
| 104 | 542 | pass # broken venv; rebuild below |
| 105 | 543 | |
| 544 | + # If a prior run installed the SDK via the --target fallback (ensurepip | |
| 545 | + # path), reuse it. Only reached when there's no working venv, so healthy | |
| 546 | + # NOOP_VENV users never pay for this probe. | |
| 547 | + if _target_sdk_importable(state_dir): | |
| 548 | + return NOOP_TARGET, "", "" | |
| 549 | + | |
| 550 | + # If a recent build was signal-killed (memory pressure), don't re-attempt | |
| 551 | + # this session — the memory-heavy venv+pip just gets killed again, burning | |
| 552 | + # the user's resources. Retry at most once per cooldown window. Reached | |
| 553 | + # only after all no-op probes, so a machine that later gets the SDK via | |
| 554 | + # system/venv/target still short-circuits above. | |
| 555 | + if _cooldown_remaining(state_dir) > 0: | |
| 556 | + return SKIP_COOLDOWN, "", "" | |
| 557 | + | |
| 106 | 558 | err_phase = "" |
| 107 | 559 | err_kind = "" |
| 108 | 560 | we_own_sentinel = False |
| @@ -125,14 +577,35 @@ | ||
| 125 | 577 | # the user's machine, pip's own default registry applies — that's the same |
| 126 | 578 | # exposure the user would have running `pip install` themselves, so |
| 127 | 579 | # we're not widening the supply-chain surface. |
| 580 | + # | |
| 581 | + # --prefer-binary: on ARM64 Windows, pip's default resolver picks a | |
| 582 | + # `cryptography` version with no published binary wheel and tries to | |
| 583 | + # build from source, which needs Rust/Cargo (almost never present | |
| 584 | + # on user machines). The build fails and the whole bootstrap returns | |
| 585 | + # BUILD_FAILED. A binary wheel exists on PyPI for an adjacent | |
| 586 | + # version (`cryptography-46.0.3-cp311-abi3-win_arm64.whl`); | |
| 587 | + # --prefer-binary tells pip to pick it. Cross-platform safe: no-op | |
| 588 | + # on platforms where the latest version already has a wheel. | |
| 128 | 589 | err_phase = "pip" |
| 590 | + # --no-cache-dir trims pip's peak memory (no cache read/write/unpack | |
| 591 | + # buffering) — helps marginal low-memory machines get under the OOM | |
| 592 | + # threshold that kills the dominant Linux builds (CCR repro). | |
| 129 | 593 | subprocess.run( |
| 130 | 594 | [str(venv_py), "-m", "pip", "install", "--quiet", |
| 131 | - "--disable-pip-version-check", "claude-agent-sdk"], | |
| 595 | + "--disable-pip-version-check", "--prefer-binary", "--no-cache-dir", | |
| 596 | + "claude-agent-sdk"], | |
| 132 | 597 | capture_output=True, timeout=120, check=True, |
| 133 | 598 | ) |
| 134 | 599 | return BUILT, "", "" |
| 135 | 600 | except subprocess.CalledProcessError as e: |
| 601 | + # Signal kill (OOM-killer SIGKILL / RLIMIT_AS SIGSEGV) — rc<0, empty | |
| 602 | + # streams. The dominant Linux failure. Record the rc, start a cooldown | |
| 603 | + # so we stop retry-storming a build that keeps getting killed, and | |
| 604 | + # skip the stderr categorization (there's nothing in stderr). err_phase | |
| 605 | + # says whether it died creating the venv or installing via pip. | |
| 606 | + if _is_signal_kill(e.returncode): | |
| 607 | + _write_cooldown(state_dir) | |
| 608 | + return BUILD_FAILED, err_phase, f"signal_killed:{e.returncode}" | |
| 136 | 609 | # Capture a stderr fingerprint so telemetry can split BUILD_FAILED by |
| 137 | 610 | # root cause (no-network, package-not-found, dns-fail, etc.). |
| 138 | 611 | # Categorize first, then keep a short raw tail for the long tail of |
| @@ -143,7 +616,34 @@ | ||
| 143 | 616 | else: |
| 144 | 617 | stderr_str = str(stderr_b) |
| 145 | 618 | s = stderr_str.lower() |
| 146 | - if "no matching distribution" in s or "could not find a version" in s: | |
| 619 | + # Venv-specific patterns checked FIRST — they overlap with some pip | |
| 620 | + # patterns (e.g. "no module named ensurepip" could match no_pip OR | |
| 621 | + # venv_ensurepip_fail; the venv-stage interpretation is the right | |
| 622 | + # one when err_phase=="venv"). Order is venv-most-specific → | |
| 623 | + # pip-historical → generic. | |
| 624 | + if err_phase == "venv" and ( | |
| 625 | + "ensurepip is not available" in s | |
| 626 | + or ("ensurepip" in s and "returned non-zero" in s) | |
| 627 | + or "the virtual environment was not created" in s and "ensurepip" in s | |
| 628 | + ): | |
| 629 | + err_kind = "venv_ensurepip_fail" | |
| 630 | + elif err_phase == "venv" and ( | |
| 631 | + "[errno 36]" in s | |
| 632 | + or "file name too long" in s | |
| 633 | + or "path too long" in s | |
| 634 | + ): | |
| 635 | + err_kind = "venv_path_too_long" | |
| 636 | + elif err_phase == "venv" and ( | |
| 637 | + "no module named venv" in s | |
| 638 | + or "no module named 'venv'" in s | |
| 639 | + ): | |
| 640 | + err_kind = "venv_no_module" | |
| 641 | + elif err_phase == "venv" and ( | |
| 642 | + "[errno 17]" in s | |
| 643 | + or ("file exists" in s and "venv" in s) | |
| 644 | + ): | |
| 645 | + err_kind = "venv_already_exists" | |
| 646 | + elif "no matching distribution" in s or "could not find a version" in s: | |
| 147 | 647 | err_kind = "pip_no_match" |
| 148 | 648 | elif "name or service not known" in s or "name resolution" in s \ |
| 149 | 649 | or "nodename nor servname" in s or "temporary failure in name" in s: |
| @@ -162,6 +662,15 @@ | ||
| 162 | 662 | err_kind = "proxy_auth" |
| 163 | 663 | elif "timeout" in s or "timed out" in s: |
| 164 | 664 | err_kind = "stderr_timeout" |
| 665 | + elif err_phase == "venv" and ( | |
| 666 | + "virtual environment was not created" in s | |
| 667 | + or "error: command" in s and "venv" in s | |
| 668 | + ): | |
| 669 | + # Generic venv-setup catch-all — matched AFTER the more specific | |
| 670 | + # venv patterns above so we don't shadow them, but BEFORE the | |
| 671 | + # other: fallback so generic venv setup failures get their own | |
| 672 | + # bucket instead of polluting the long-tail signature space. | |
| 673 | + err_kind = "venv_setup_failed" | |
| 165 | 674 | else: |
| 166 | 675 | # First 60 chars of the last non-empty stderr line — bounded to |
| 167 | 676 | # stay inside CC's metric value-length budget. Real failure modes |
| @@ -171,10 +680,27 @@ | ||
| 171 | 680 | "", |
| 172 | 681 | )[:60] |
| 173 | 682 | err_kind = f"other:{tail}" if tail else "other" |
| 683 | + # venv couldn't bootstrap pip (ensurepip missing) but pip itself may | |
| 684 | + # work — fall back to a flat `pip install --target`. Only this one | |
| 685 | + # category falls through; every other venv/pip failure is terminal. | |
| 686 | + # The finally block unlinks our sentinel first (so the target build | |
| 687 | + # isn't blocked by it); _build_via_target does the target install. | |
| 688 | + if err_kind == "venv_ensurepip_fail": | |
| 689 | + if we_own_sentinel: | |
| 690 | + sentinel.unlink(missing_ok=True) | |
| 691 | + we_own_sentinel = False | |
| 692 | + return _build_via_target(state_dir) | |
| 174 | 693 | return BUILD_FAILED, err_phase, err_kind |
| 175 | 694 | except subprocess.TimeoutExpired: |
| 176 | 695 | return BUILD_FAILED, err_phase, "subprocess_timeout" |
| 177 | 696 | except Exception as e: |
| 697 | + # Embed errno for OSError-family exceptions ("exc:OSError:28") so | |
| 698 | + # telemetry can decode the OS-level cause (ENOENT/EACCES/ENOSPC/…), | |
| 699 | + # not just the Python class. #2154 follow-up: this is the dominant | |
| 700 | + # remaining venv BUILD_FAILED bucket. See _encode_exc_kind/_encode_errno. | |
| 701 | + errno = getattr(e, "errno", None) | |
| 702 | + if isinstance(errno, int): | |
| 703 | + return BUILD_FAILED, err_phase, f"exc:{type(e).__name__}:{errno}" | |
| 178 | 704 | return BUILD_FAILED, err_phase, f"exc:{type(e).__name__}" |
| 179 | 705 | finally: |
| 180 | 706 | # Only remove the sentinel if THIS process created it. The |
| @@ -186,6 +712,53 @@ | ||
| 186 | 712 | sentinel.unlink(missing_ok=True) |
| 187 | 713 | |
| 188 | 714 | |
| 715 | +def _maybe_emit_user_notice(outcome: int, pv: int) -> str | None: | |
| 716 | + """Return a one-time user-visible notice when the agentic reviewer is | |
| 717 | + in a persistent broken state on this machine, or None if we've already | |
| 718 | + shown the notice for this plugin version (or shouldn't show one). | |
| 719 | + | |
| 720 | + The marker file is plugin-version-keyed: a future plugin update can | |
| 721 | + re-notify if behavior changes (e.g. we ship out-of-process SDK in v3 | |
| 722 | + and want to tell affected users it's fixed). Failures to write the | |
| 723 | + marker degrade to "skip the notice this session" so we don't spam | |
| 724 | + every SessionStart on a read-only home dir. | |
| 725 | + | |
| 726 | + Currently only HOOK_PY_INCOMPATIBLE qualifies. BUILD_FAILED is | |
| 727 | + intentionally excluded — it covers transient causes (network failure, | |
| 728 | + pip registry hiccup, in-flight rebuild) where the next session may | |
| 729 | + succeed and a permanent notice would mislead. | |
| 730 | + """ | |
| 731 | + if outcome != HOOK_PY_INCOMPATIBLE: | |
| 732 | + return None | |
| 733 | + try: | |
| 734 | + state_dir = Path(_resolve_state_dir()) | |
| 735 | + marker = state_dir / f".agentic_unavailable_notice_v{pv or 0}" | |
| 736 | + if marker.exists(): | |
| 737 | + return None | |
| 738 | + state_dir.mkdir(parents=True, exist_ok=True) | |
| 739 | + # Write timestamp + Python version so the marker is self-documenting | |
| 740 | + # if a user goes looking. O_EXCL would be racier with no real win | |
| 741 | + # (two concurrent SessionStarts both showing the notice once is fine). | |
| 742 | + marker.write_text( | |
| 743 | + f"{time.strftime('%Y-%m-%dT%H:%M:%SZ', time.gmtime())} " | |
| 744 | + f"py={sys.version_info[0]}.{sys.version_info[1]}\n" | |
| 745 | + ) | |
| 746 | + except OSError: | |
| 747 | + return None | |
| 748 | + return ( | |
| 749 | + f"⚠ security-guidance plugin: the cross-file commit reviewer " | |
| 750 | + f"(layer 3 of 3 — catches IDOR, auth-bypass, cross-file SSRF) " | |
| 751 | + f"is unavailable in this environment. It requires Python ≥3.10, " | |
| 752 | + f"but the hook is running on " | |
| 753 | + f"{sys.version_info[0]}.{sys.version_info[1]}.\n\n" | |
| 754 | + f"Pattern checks and the single-shot LLM diff review are still " | |
| 755 | + f"active. To enable the deeper reviewer, install Python 3.10+ " | |
| 756 | + f"(e.g. `brew install python` on macOS) and restart Claude Code.\n\n" | |
| 757 | + f"This notice is shown once per plugin version. " | |
| 758 | + f"See: github.com/anthropics/claude-plugins-official/issues/2071" | |
| 759 | + ) | |
| 760 | + | |
| 761 | + | |
| 189 | 762 | if __name__ == "__main__": |
| 190 | 763 | # Tell the harness this is async — venv create + pip install can take |
| 191 | 764 | # 30-60s on a cold cache, well past the default sync hook timeout. |
| @@ -204,22 +777,86 @@ | ||
| 204 | 777 | # and takes the FIRST non-{"async":...} JSON line as the hook response; |
| 205 | 778 | # its `metrics` key is forwarded to the hook metrics event on the |
| 206 | 779 | # next attachments pass. Must be a single line — the registry splits on |
| 207 | - # \n and json-parses each independently. Values must be bool|number OR | |
| 208 | - # short strings (CC accepts string metric values if they're not | |
| 209 | - # null). Stay inside the 10-key emit cap. | |
| 780 | + # \n and json-parses each independently. | |
| 781 | + # | |
| 782 | + # IMPORTANT — values must be bool|finite-number. The validation comment | |
| 783 | + # has historically said "or short strings" but that was wrong: CC's | |
| 784 | + # plugin-metrics pipeline silently drops plugin-emitted string values. | |
| 785 | + # Stay inside the 10-key emit cap. | |
| 210 | 786 | metrics: dict[str, object] = { |
| 211 | 787 | "sdk_bootstrap": outcome, |
| 212 | 788 | "sdk_bootstrap_ms": round((time.perf_counter() - t0) * 1000), |
| 213 | 789 | } |
| 214 | 790 | if err_kind: |
| 215 | - # Truncate defensively; categorized values are <40 chars but the | |
| 216 | - # `other:<tail>` mode could be longer. err_phase may be empty for | |
| 217 | - # pre-venv failures (state_dir.mkdir perm-denied, sentinel O_EXCL | |
| 218 | - # raising a non-FileExistsError OSError) — emit as "pre" so the | |
| 219 | - # err_kind isn't silently dropped. | |
| 220 | - metrics["sdk_bootstrap_phase"] = (err_phase or "pre")[:16] | |
| 221 | - metrics["sdk_bootstrap_err"] = err_kind[:96] | |
| 791 | + # Encode phase + err_kind as integer codes (see | |
| 792 | + # SDK_BOOTSTRAP_PHASE_CODES / SDK_BOOTSTRAP_ERR_CODES). Earlier | |
| 793 | + # versions emitted these as strings and CC dropped them — restoring | |
| 794 | + # the diagnostic split that 28K BUILD_FAILED/day need to triage by | |
| 795 | + # root cause. err_phase defaults to "pre" when empty (pre-venv | |
| 796 | + # failure path, e.g. state_dir.mkdir perm-denied). | |
| 797 | + metrics["sdk_bootstrap_phase"] = _encode_phase(err_phase or "pre") | |
| 798 | + metrics["sdk_bootstrap_err"] = _encode_err_kind(err_kind) | |
| 799 | + # For "other:<tail>" (encoded err==99), emit a bounded integer | |
| 800 | + # hash of the stderr tail so BQ can distinguish patterns inside | |
| 801 | + # the _uncategorized bucket without unbounded cardinality. Zero | |
| 802 | + # when err_kind is categorized — the schema reader treats 0 as | |
| 803 | + # "no signal", matching the absence convention. | |
| 804 | + sig = _encode_stderr_sig(err_kind) | |
| 805 | + if sig: | |
| 806 | + metrics["sdk_bootstrap_stderr_sig"] = sig | |
| 807 | + # Exception-type + errno for the "exc:" bucket (the dominant | |
| 808 | + # remaining venv BUILD_FAILED mode per #2154 telemetry). Both | |
| 809 | + # auto-omit (0) on stderr/categorized failures. | |
| 810 | + exc = _encode_exc_kind(err_kind) | |
| 811 | + if exc: | |
| 812 | + metrics["sdk_bootstrap_exc"] = exc | |
| 813 | + exc_errno = _encode_errno(err_kind) | |
| 814 | + if exc_errno: | |
| 815 | + metrics["sdk_bootstrap_errno"] = exc_errno | |
| 816 | + # Subprocess returncode for signal kills (-9 OOM-killer / -11 | |
| 817 | + # RLIMIT_AS / -6 abort). Confirms in prod which signal dominates the | |
| 818 | + # Linux memory-kill bucket. 0 (omitted) for non-signal failures. | |
| 819 | + rc = _encode_rc(err_kind) | |
| 820 | + if rc: | |
| 821 | + metrics["sdk_bootstrap_rc"] = rc | |
| 822 | + # venv_ensurepip_fail (code 11) is the top categorizable venv | |
| 823 | + # failure, and telemetry shows it's NOT just Debian — macOS has the | |
| 824 | + # most distinct affected users. Probe whether this interpreter has | |
| 825 | + # pip so we know if a `pip install --target` fallback (Option A) | |
| 826 | + # would actually help, vs the user needing a system package. Probed | |
| 827 | + # only here (not on the happy path) to avoid an extra subprocess | |
| 828 | + # per healthy session. | |
| 829 | + if _encode_err_kind(err_kind) == 11: | |
| 830 | + metrics["sdk_has_pip"] = _probe_has_pip() | |
| 831 | + # When the hook interpreter is <3.10 (HOOK_PY_INCOMPATIBLE), probe for a | |
| 832 | + # 3.10+ interpreter at known non-PATH locations. Non-zero sdk_alt_py = | |
| 833 | + # this user is RECOVERABLE by an explicit-path search in sg-python.sh; 0 = | |
| 834 | + # genuinely 3.9-only (needs a user install). Sizes the macOS Py-3.9 cohort | |
| 835 | + # (~13.6% of macOS sessions) before we build the search. Incompatible path | |
| 836 | + # only — healthy sessions never run it. | |
| 837 | + if outcome == HOOK_PY_INCOMPATIBLE: | |
| 838 | + metrics["sdk_alt_py"] = _probe_alt_python() | |
| 839 | + # Interpreter version (major*100 + minor, e.g. 309 / 312), emitted on | |
| 840 | + # every bootstrap. Disambiguates the macOS cohort (Apple 3.9 vs a 3.10+ | |
| 841 | + # with broken ensurepip) for both venv_ensurepip_fail AND | |
| 842 | + # HOOK_PY_INCOMPATIBLE (whose "py_3.9" err_kind otherwise collapses to | |
| 843 | + # err=99, losing the version). Cheap — no subprocess, just sys.version_info. | |
| 844 | + metrics["sdk_hook_py"] = sys.version_info[0] * 100 + sys.version_info[1] | |
| 222 | 845 | pv = _plugin_version_int() |
| 223 | 846 | if pv: |
| 224 | 847 | metrics["pv"] = pv |
| 225 | - print(json.dumps({"metrics": metrics}), flush=True) | |
| 848 | + response: dict[str, object] = {"metrics": metrics} | |
| 849 | + # One-time user-visible notice when the agentic reviewer is dead on | |
| 850 | + # arrival. Uses hookSpecificOutput.additionalContext (SessionStart's | |
| 851 | + # supported channel for surfacing text to both the model and the user) | |
| 852 | + # plus systemMessage as a belt-and-suspenders. Marker-file-gated so | |
| 853 | + # this fires exactly once per plugin version per install — see | |
| 854 | + # _maybe_emit_user_notice. | |
| 855 | + notice = _maybe_emit_user_notice(outcome, pv) | |
| 856 | + if notice: | |
| 857 | + response["hookSpecificOutput"] = { | |
| 858 | + "hookEventName": "SessionStart", | |
| 859 | + "additionalContext": notice, | |
| 860 | + } | |
| 861 | + response["systemMessage"] = notice | |
| 862 | + print(json.dumps(response), flush=True) |
| @@ -26,18 +26,69 @@ | ||
| 26 | 26 | "git", |
| 27 | 27 | "-c", "core.fsmonitor=false", |
| 28 | 28 | "-c", "core.hooksPath=/dev/null", |
| 29 | + # core.quotePath=false: emit raw UTF-8 in path-emitting commands instead | |
| 30 | + # of C-quoting non-ASCII bytes (default `"\\303\\201vila/..."` vs | |
| 31 | + # `Ávila/...`). Downstream parsers — both ours (parse_diff_into_files, | |
| 32 | + # extract_file_paths_from_diff) and Python stdlib (os.path.isabs, | |
| 33 | + # os.path.join) — expect raw paths and silently drop / mishandle the | |
| 34 | + # quoted form. Adding the flag globally to GIT_CMD covers every | |
| 35 | + # subprocess.run site that uses the splat — diff feeders, rev-parse | |
| 36 | + # path queries (--show-toplevel, --git-dir, --git-common-dir), | |
| 37 | + # reflog %gs subjects, ls-files, status, etc. — without per-site | |
| 38 | + # flag duplication. See #2082, #2099. | |
| 39 | + "-c", "core.quotePath=false", | |
| 40 | + # A work-tree `git diff` that meets stat-stale entries (mtime changed, | |
| 41 | + # content same) rewrites the index under index.lock, GIT_OPTIONAL_LOCKS or | |
| 42 | + # not. Off, every diff here only reads the index, so none needs a copy. | |
| 43 | + "-c", "diff.autoRefreshIndex=false", | |
| 29 | 44 | ] |
| 30 | 45 | |
| 46 | +SAFE_GIT_CONFIG = ( | |
| 47 | + ("core.fsmonitor", "false"), | |
| 48 | + ("core.hooksPath", "/dev/null"), | |
| 49 | +) | |
| 31 | 50 | |
| 51 | +# The hook's git runs in the background beside the session's own git. Without | |
| 52 | +# this, `git status` holds .git/index.lock while it writes back a refreshed | |
| 53 | +# index: a concurrent `git add` fails, and a timeout kill leaves the lock. | |
| 54 | +SAFE_GIT_ENV = { | |
| 55 | + "GIT_OPTIONAL_LOCKS": "0", | |
| 56 | +} | |
| 57 | + | |
| 58 | + | |
| 59 | +def git_config_env(pairs, base=None): | |
| 60 | + base = os.environ if base is None else base | |
| 61 | + try: | |
| 62 | + n = max(0, int(base.get("GIT_CONFIG_COUNT") or 0)) | |
| 63 | + except (TypeError, ValueError): | |
| 64 | + n = 0 | |
| 65 | + env = {} | |
| 66 | + for i, (k, v) in enumerate(pairs, start=n): | |
| 67 | + env[f"GIT_CONFIG_KEY_{i}"] = k | |
| 68 | + env[f"GIT_CONFIG_VALUE_{i}"] = v | |
| 69 | + env["GIT_CONFIG_COUNT"] = str(n + len(pairs)) | |
| 70 | + return env | |
| 71 | + | |
| 72 | + | |
| 73 | +def apply_safe_git_env(): | |
| 74 | + os.environ.update(git_config_env(SAFE_GIT_CONFIG)) | |
| 75 | + os.environ.update(SAFE_GIT_ENV) | |
| 76 | + | |
| 77 | + | |
| 32 | 78 | def _git_rev_parse_head(cwd): |
| 33 | 79 | """Return the current HEAD SHA, or None if not a git repo / no commits.""" |
| 34 | 80 | try: |
| 81 | + # See #2099: text=True on Windows cp1252 crashes the reader thread on | |
| 82 | + # any UTF-8 byte undefined in cp1252 (e.g. via a git error message | |
| 83 | + # referencing a non-ASCII filename in stderr). stdout is a SHA so it | |
| 84 | + # IS safe; stderr is not. capture_output=True with bytes-by-default | |
| 85 | + # never decodes, so the reader thread can't crash. | |
| 35 | 86 | result = subprocess.run( |
| 36 | 87 | [*GIT_CMD, "rev-parse", "HEAD"], |
| 37 | - cwd=cwd, capture_output=True, text=True, timeout=5 | |
| 88 | + cwd=cwd, capture_output=True, timeout=5 | |
| 38 | 89 | ) |
| 39 | 90 | if result.returncode == 0 and result.stdout.strip(): |
| 40 | - return result.stdout.strip() | |
| 91 | + return result.stdout.decode("utf-8", errors="replace").strip() | |
| 41 | 92 | return None |
| 42 | 93 | except (subprocess.TimeoutExpired, FileNotFoundError, OSError): |
| 43 | 94 | return None |
| @@ -52,13 +103,17 @@ | ||
| 52 | 103 | Returns the absolute path to the index file, or None. |
| 53 | 104 | """ |
| 54 | 105 | try: |
| 106 | + # See #2099: stdout here is a PATH which can contain non-ASCII bytes | |
| 107 | + # (e.g. C:\אבטחה\repo\.git). text=True decodes via cp1252 strict on | |
| 108 | + # Windows → crashes the reader thread → returns stdout=None → | |
| 109 | + # caller does .strip() on None → AttributeError. Decode manually. | |
| 55 | 110 | result = subprocess.run( |
| 56 | 111 | [*GIT_CMD, "rev-parse", "--git-dir"], |
| 57 | - cwd=cwd, capture_output=True, text=True, timeout=5 | |
| 112 | + cwd=cwd, capture_output=True, timeout=5 | |
| 58 | 113 | ) |
| 59 | 114 | if result.returncode != 0: |
| 60 | 115 | return None |
| 61 | - git_dir = result.stdout.strip() | |
| 116 | + git_dir = result.stdout.decode("utf-8", errors="replace").strip() | |
| 62 | 117 | if not os.path.isabs(git_dir): |
| 63 | 118 | git_dir = os.path.join(cwd, git_dir) |
| 64 | 119 | index_path = os.path.join(git_dir, "index") |
| @@ -88,31 +143,295 @@ | ||
| 88 | 143 | return ["--"] + rel if rel else [] |
| 89 | 144 | |
| 90 | 145 | |
| 146 | +_TEMP_INDEX_PREFIX = "security_hook_idx_" | |
| 147 | +# A copy is named <prefix>pid<hook pid>_<8 random chars>, so a later run can | |
| 148 | +# tell that the hook which made it is gone. Names from earlier releases have | |
| 149 | +# only the 8 random characters and never match. | |
| 150 | +_OWNER_PID = re.compile(re.escape(_TEMP_INDEX_PREFIX) + r"pid(\d+)_[^.]{8}(?:\.|$)") | |
| 151 | +# A live copy is in use for under a minute (a couple of bounded git calls), so | |
| 152 | +# anything this old belongs to a run that was killed. | |
| 153 | +_TEMP_INDEX_STALE_S = 1800 | |
| 154 | +# Unlinking index-sized files is slow; a large backlog drains over several | |
| 155 | +# calls instead of stalling one hook past its timeout. | |
| 156 | +_SWEEP_BUDGET_S = 2 | |
| 157 | + | |
| 158 | + | |
| 159 | +def _owner_is_gone(name): | |
| 160 | + """True when `name` records the pid of the hook that made it and no such | |
| 161 | + process exists. A reused pid reads as alive: the file then waits for the | |
| 162 | + age limit. Never probes on Windows, where os.kill would end the process.""" | |
| 163 | + if not hasattr(os, "getuid"): | |
| 164 | + return False | |
| 165 | + m = _OWNER_PID.match(name) | |
| 166 | + if not m: | |
| 167 | + return False | |
| 168 | + try: | |
| 169 | + os.kill(int(m.group(1)), 0) | |
| 170 | + except ProcessLookupError: | |
| 171 | + return True | |
| 172 | + except (OSError, OverflowError, ValueError): | |
| 173 | + return False | |
| 174 | + return False | |
| 175 | + | |
| 176 | + | |
| 177 | +def _sweep_stale_indexes(dirpath, uid): | |
| 178 | + """Delete index copies in `dirpath` that a killed run left behind. Only | |
| 179 | + regular files owned by `uid` (None: no ownership check) are touched. | |
| 180 | + | |
| 181 | + A copy goes as soon as the hook that made it is gone: a killed hook can | |
| 182 | + leave one per run, and index-sized files fill a disk long before the age | |
| 183 | + limit. The age limit covers names without a pid and reused pids. | |
| 184 | + | |
| 185 | + Age is the newer of mtime and ctime: a copy keeps .git/index's mtime, | |
| 186 | + which may be days old, so mtime alone would delete a file that another | |
| 187 | + running hook is still using.""" | |
| 188 | + import stat | |
| 189 | + import time | |
| 190 | + cutoff = time.time() - _TEMP_INDEX_STALE_S | |
| 191 | + deadline = time.monotonic() + _SWEEP_BUDGET_S | |
| 192 | + try: | |
| 193 | + names = os.listdir(dirpath) | |
| 194 | + except OSError: | |
| 195 | + return | |
| 196 | + for name in names: | |
| 197 | + if not name.startswith(_TEMP_INDEX_PREFIX): | |
| 198 | + continue | |
| 199 | + p = os.path.join(dirpath, name) | |
| 200 | + try: | |
| 201 | + st = os.lstat(p) | |
| 202 | + if not stat.S_ISREG(st.st_mode): | |
| 203 | + continue | |
| 204 | + if uid is not None and st.st_uid != uid: | |
| 205 | + continue | |
| 206 | + if max(st.st_mtime, st.st_ctime) < cutoff or _owner_is_gone(name): | |
| 207 | + os.unlink(p) | |
| 208 | + if time.monotonic() > deadline: | |
| 209 | + return | |
| 210 | + except OSError: | |
| 211 | + pass | |
| 212 | + | |
| 213 | + | |
| 214 | +def _sweep_stale_stash_indexes(real_index): | |
| 215 | + """Delete `<index>.stash.<pid>` files a killed `git stash create` left | |
| 216 | + next to the real index. Releases before 2.0.10 ran it there, so a clone | |
| 217 | + can hold hundreds, each the size of the index, and nothing else removes | |
| 218 | + them. Only our own regular files past the age limit go: git keeps one for | |
| 219 | + seconds. `index.lock` is never touched; a live git may hold it.""" | |
| 220 | + import stat | |
| 221 | + import time | |
| 222 | + uid = os.getuid() if hasattr(os, "getuid") else None | |
| 223 | + cutoff = time.time() - _TEMP_INDEX_STALE_S | |
| 224 | + deadline = time.monotonic() + _SWEEP_BUDGET_S | |
| 225 | + dirpath = os.path.dirname(real_index) | |
| 226 | + stale = re.compile(re.escape(os.path.basename(real_index)) + r"\.stash\.\d+$") | |
| 227 | + try: | |
| 228 | + names = os.listdir(dirpath) | |
| 229 | + except OSError: | |
| 230 | + return | |
| 231 | + for name in names: | |
| 232 | + if not stale.match(name): | |
| 233 | + continue | |
| 234 | + p = os.path.join(dirpath, name) | |
| 235 | + try: | |
| 236 | + st = os.lstat(p) | |
| 237 | + if not stat.S_ISREG(st.st_mode): | |
| 238 | + continue | |
| 239 | + if uid is not None and st.st_uid != uid: | |
| 240 | + continue | |
| 241 | + if max(st.st_mtime, st.st_ctime) < cutoff: | |
| 242 | + os.unlink(p) | |
| 243 | + if time.monotonic() > deadline: | |
| 244 | + return | |
| 245 | + except OSError: | |
| 246 | + pass | |
| 247 | + | |
| 248 | + | |
| 249 | +# Index copies this process has made and not yet removed. | |
| 250 | +_live_copies = set() | |
| 251 | + | |
| 252 | +# A hook working on a copy needs up to three times the index's size for a | |
| 253 | +# moment (the copy, and git's rewrite and stash index next to it). With many | |
| 254 | +# agents in one clone that alone can fill a disk, so only so many hooks per | |
| 255 | +# user work on a copy at once; the others wait, then go without. | |
| 256 | +_MAX_LIVE_COPIES = 8 | |
| 257 | +_SLOT_WAIT_S = 5 | |
| 258 | + | |
| 259 | + | |
| 260 | +def _copy_slots(tmpdir, index_bytes): | |
| 261 | + """How many hooks may work on a copy at once: what fits into a quarter of | |
| 262 | + the free space at three times the index each, from 1 to | |
| 263 | + _MAX_LIVE_COPIES.""" | |
| 264 | + import shutil | |
| 265 | + try: | |
| 266 | + free = shutil.disk_usage(tmpdir).free | |
| 267 | + except OSError: | |
| 268 | + return 1 | |
| 269 | + return max(1, min(_MAX_LIVE_COPIES, free // (12 * max(index_bytes, 1)))) | |
| 270 | + | |
| 271 | + | |
| 91 | 272 | @contextlib.contextmanager |
| 273 | +def _copy_slot(tmpdir, index_bytes=0): | |
| 274 | + """Hold one of the copy slots; yields False when none came free in | |
| 275 | + _SLOT_WAIT_S. A slot is an flock, so the kernel frees it when its holder | |
| 276 | + dies, however it dies. No cap without flock or a private dir.""" | |
| 277 | + import time | |
| 278 | + try: | |
| 279 | + import fcntl | |
| 280 | + except ImportError: | |
| 281 | + fcntl = None | |
| 282 | + if fcntl is None or tmpdir is None: | |
| 283 | + yield True | |
| 284 | + return | |
| 285 | + deadline = time.monotonic() + _SLOT_WAIT_S | |
| 286 | + held = None | |
| 287 | + while held is None: | |
| 288 | + for i in range(_copy_slots(tmpdir, index_bytes)): | |
| 289 | + try: | |
| 290 | + fd = os.open(os.path.join(tmpdir, f"slot{i}.lock"), | |
| 291 | + os.O_RDWR | os.O_CREAT, 0o600) | |
| 292 | + except OSError: | |
| 293 | + continue | |
| 294 | + try: | |
| 295 | + fcntl.flock(fd, fcntl.LOCK_EX | fcntl.LOCK_NB) | |
| 296 | + held = fd | |
| 297 | + break | |
| 298 | + except OSError: | |
| 299 | + os.close(fd) | |
| 300 | + if held is None: | |
| 301 | + if time.monotonic() >= deadline: | |
| 302 | + break | |
| 303 | + time.sleep(0.1) | |
| 304 | + try: | |
| 305 | + yield held is not None | |
| 306 | + finally: | |
| 307 | + if held is not None: | |
| 308 | + os.close(held) | |
| 309 | + | |
| 310 | + | |
| 311 | +def _remove_copy(tmp_index): | |
| 312 | + """Remove an index copy and the files git wrote next to it.""" | |
| 313 | + import glob | |
| 314 | + for p in glob.glob(glob.escape(tmp_index) + "*"): | |
| 315 | + try: | |
| 316 | + os.unlink(p) | |
| 317 | + except OSError: | |
| 318 | + pass | |
| 319 | + | |
| 320 | + | |
| 321 | +def remove_copies_on_termination(): | |
| 322 | + """Remove this process's index copies when it is told to terminate. | |
| 323 | + Python's default SIGTERM action ends the process without running | |
| 324 | + `finally`, which left an index-sized file behind each time.""" | |
| 325 | + import signal | |
| 326 | + | |
| 327 | + def _on_term(signum, _frame): | |
| 328 | + for tmp_index in list(_live_copies): | |
| 329 | + _remove_copy(tmp_index) | |
| 330 | + os._exit(128 + signum) | |
| 331 | + | |
| 332 | + try: | |
| 333 | + signal.signal(signal.SIGTERM, _on_term) | |
| 334 | + except (ValueError, OSError, AttributeError): | |
| 335 | + pass # not the main thread, or no SIGTERM here | |
| 336 | + | |
| 337 | + | |
| 338 | +def _hook_tmpdir(): | |
| 339 | + """Private per-user directory for throwaway index copies, or None to use | |
| 340 | + the bare temp dir. | |
| 341 | + | |
| 342 | + An index copy can be hundreds of MB and a killed hook cannot clean up, so | |
| 343 | + copies live in one 0700 directory and anything stale is swept on entry, | |
| 344 | + here and in the bare temp dir. The name is predictable and the temp dir | |
| 345 | + may be shared, so anything that is not a real directory we own is | |
| 346 | + refused. Windows has no uid and its temp dir is already per-user.""" | |
| 347 | + import stat | |
| 348 | + import tempfile | |
| 349 | + base = tempfile.gettempdir() | |
| 350 | + uid = os.getuid() if hasattr(os, "getuid") else None | |
| 351 | + _sweep_stale_indexes(base, uid) | |
| 352 | + if uid is None: | |
| 353 | + return None | |
| 354 | + d = os.path.join(base, f"claude-security-guidance-{uid}") | |
| 355 | + try: | |
| 356 | + os.mkdir(d, 0o700) | |
| 357 | + except FileExistsError: | |
| 358 | + pass | |
| 359 | + except OSError: | |
| 360 | + return None | |
| 361 | + try: | |
| 362 | + st = os.lstat(d) | |
| 363 | + except OSError: | |
| 364 | + return None | |
| 365 | + if not stat.S_ISDIR(st.st_mode) or st.st_uid != uid: | |
| 366 | + return None | |
| 367 | + try: | |
| 368 | + os.chmod(d, 0o700) | |
| 369 | + except OSError: | |
| 370 | + pass | |
| 371 | + _sweep_stale_indexes(d, uid) | |
| 372 | + return d | |
| 373 | + | |
| 374 | + | |
| 375 | +@contextlib.contextmanager | |
| 92 | 376 | def _temp_index(cwd, untracked_paths=None): |
| 93 | 377 | """Yield an env dict pointing GIT_INDEX_FILE at a throwaway copy of the |
| 94 | 378 | repo's index with `git add --intent-to-add` applied, so untracked files |
| 95 | 379 | show up in subsequent `git diff` calls without touching the user's real |
| 96 | - index. Yields None if no index can be found (bare repo / not a repo); the | |
| 97 | - caller should fall back to a plain diff. Always cleans up the temp file. | |
| 380 | + index. Yields None if no index can be found (bare repo / not a repo) or | |
| 381 | + no copy slot came free; the caller should fall back to a plain diff. | |
| 382 | + Always cleans up the temp file. | |
| 98 | 383 | |
| 99 | 384 | Perf: when `untracked_paths` is given, only those paths are added (O(n) |
| 100 | 385 | in untracked count). The default `add -N .` stats every file in the |
| 101 | 386 | worktree — slow in large repos vs fast targeted scan. v2 callers |
| 102 | 387 | already know the untracked set from `git status --porcelain`, so they |
| 103 | - pass it; v1 keeps the whole-tree scan since it has no prior list.""" | |
| 104 | - import shutil | |
| 105 | - import tempfile | |
| 388 | + pass it; v1 keeps the whole-tree scan since it has no prior list. | |
| 106 | 389 | |
| 390 | + Cleanup removes `<tmp_index>*`, not just the copy: git writes | |
| 391 | + `<index>.lock` and `<index>.stash.<pid>` next to the index it is given and | |
| 392 | + cannot remove them itself when a subprocess timeout kills it.""" | |
| 107 | 393 | real_index = _find_git_index(cwd) |
| 108 | 394 | if not real_index: |
| 109 | 395 | yield None |
| 110 | 396 | return |
| 111 | 397 | |
| 112 | - tmp_fd, tmp_index = tempfile.mkstemp(prefix="security_hook_idx_") | |
| 398 | + _sweep_stale_stash_indexes(real_index) | |
| 399 | + tmpdir = _hook_tmpdir() | |
| 400 | + try: | |
| 401 | + index_bytes = os.path.getsize(real_index) | |
| 402 | + except OSError: | |
| 403 | + index_bytes = 0 | |
| 404 | + with _copy_slot(tmpdir, index_bytes) as got_slot: | |
| 405 | + if not got_slot: | |
| 406 | + debug_log("no index copy slot came free; going without a copy") | |
| 407 | + yield None | |
| 408 | + return | |
| 409 | + with _copied_index(cwd, real_index, tmpdir, untracked_paths) as env: | |
| 410 | + yield env | |
| 411 | + | |
| 412 | + | |
| 413 | +@contextlib.contextmanager | |
| 414 | +def _copied_index(cwd, real_index, tmpdir, untracked_paths): | |
| 415 | + import shutil | |
| 416 | + import tempfile | |
| 417 | + | |
| 418 | + tmp_fd, tmp_index = tempfile.mkstemp( | |
| 419 | + prefix=f"{_TEMP_INDEX_PREFIX}pid{os.getpid()}_", dir=tmpdir) | |
| 113 | 420 | os.close(tmp_fd) |
| 421 | + _live_copies.add(tmp_index) | |
| 114 | 422 | try: |
| 115 | - shutil.copy2(real_index, tmp_index) | |
| 423 | + # A hard link where the filesystem allows it: no bytes are copied, and | |
| 424 | + # git never writes an index in place (it renames a new file over it), | |
| 425 | + # so the real index cannot change through the link. Either way the | |
| 426 | + # copy must keep .git/index's mtime (copy2, not copyfile): git | |
| 427 | + # re-reads any file whose mtime is not older than the index file's, | |
| 428 | + # and with a fresh mtime it would miss a same-size edit made in the | |
| 429 | + # same second as the last index write. | |
| 430 | + try: | |
| 431 | + os.unlink(tmp_index) | |
| 432 | + os.link(real_index, tmp_index) | |
| 433 | + except (OSError, NotImplementedError, AttributeError): | |
| 434 | + shutil.copy2(real_index, tmp_index) | |
| 116 | 435 | env = {**os.environ, "GIT_INDEX_FILE": tmp_index} |
| 117 | 436 | if untracked_paths is None: |
| 118 | 437 | add_args = ["."] |
| @@ -128,27 +447,35 @@ | ||
| 128 | 447 | else: |
| 129 | 448 | add_args = None |
| 130 | 449 | if add_args: |
| 450 | + # No stdout used here (only returncode matters), but text=True | |
| 451 | + # still spawns reader threads that decode stderr — git error | |
| 452 | + # messages can reference non-ASCII filenames and crash on | |
| 453 | + # cp1252. See #2099. Drop text=True so bytes stay raw. | |
| 131 | 454 | subprocess.run( |
| 132 | 455 | [*GIT_CMD, "add", "--intent-to-add"] + add_args, |
| 133 | - cwd=cwd, capture_output=True, text=True, timeout=10, | |
| 456 | + cwd=cwd, capture_output=True, timeout=10, | |
| 134 | 457 | env=env, |
| 135 | 458 | ) |
| 136 | 459 | yield env |
| 137 | 460 | finally: |
| 138 | - try: | |
| 139 | - os.unlink(tmp_index) | |
| 140 | - except OSError: | |
| 141 | - pass | |
| 461 | + _remove_copy(tmp_index) | |
| 462 | + _live_copies.discard(tmp_index) | |
| 142 | 463 | |
| 143 | 464 | |
| 144 | 465 | def _git_toplevel(cwd): |
| 145 | 466 | """Absolute repo root for `cwd`, or None if not in a work tree.""" |
| 146 | 467 | try: |
| 468 | + # See #2099: stdout is a PATH — `C:\אבטחה\repo` returned as UTF-8 | |
| 469 | + # bytes by git. text=True would decode via cp1252 strict on Windows | |
| 470 | + # → reader-thread crash. Decode manually with errors="replace". | |
| 147 | 471 | r = subprocess.run( |
| 148 | 472 | [*GIT_CMD, "rev-parse", "--show-toplevel"], |
| 149 | - cwd=cwd, capture_output=True, text=True, timeout=5, | |
| 473 | + cwd=cwd, capture_output=True, timeout=5, | |
| 150 | 474 | ) |
| 151 | - return r.stdout.strip() if r.returncode == 0 and r.stdout.strip() else None | |
| 475 | + if r.returncode != 0: | |
| 476 | + return None | |
| 477 | + path = r.stdout.decode("utf-8", errors="replace").strip() | |
| 478 | + return path if path else None | |
| 152 | 479 | except (subprocess.TimeoutExpired, FileNotFoundError, OSError): |
| 153 | 480 | return None |
| 154 | 481 | |
| @@ -164,13 +491,15 @@ | ||
| 164 | 491 | callers can degrade (push-sweep state is best-effort). |
| 165 | 492 | """ |
| 166 | 493 | try: |
| 494 | + # See #2099: stdout is a PATH (shared gitdir), may be non-ASCII. | |
| 495 | + # Decode bytes manually to avoid cp1252 reader-thread crash. | |
| 167 | 496 | r = subprocess.run( |
| 168 | 497 | [*GIT_CMD, "rev-parse", "--git-common-dir"], |
| 169 | - cwd=repo_root, capture_output=True, text=True, timeout=5, | |
| 498 | + cwd=repo_root, capture_output=True, timeout=5, | |
| 170 | 499 | ) |
| 171 | 500 | if r.returncode != 0: |
| 172 | 501 | return None |
| 173 | - d = r.stdout.strip() | |
| 502 | + d = r.stdout.decode("utf-8", errors="replace").strip() | |
| 174 | 503 | return d if os.path.isabs(d) else os.path.join(repo_root, d) |
| 175 | 504 | except (subprocess.TimeoutExpired, FileNotFoundError, OSError): |
| 176 | 505 | return None |
| @@ -179,13 +508,15 @@ | ||
| 179 | 508 | def _git_rev_list_range(repo_root, base, head="HEAD"): |
| 180 | 509 | """Shas in `base..head`, oldest→newest. Empty list on error.""" |
| 181 | 510 | try: |
| 511 | + # See #2099: stdout is ASCII SHAs, but stderr can carry git error | |
| 512 | + # messages referencing non-ASCII filenames — keep bytes raw. | |
| 182 | 513 | r = subprocess.run( |
| 183 | 514 | [*GIT_CMD, "rev-list", "--reverse", f"{base}..{head}"], |
| 184 | - cwd=repo_root, capture_output=True, text=True, timeout=10, | |
| 515 | + cwd=repo_root, capture_output=True, timeout=10, | |
| 185 | 516 | ) |
| 186 | 517 | if r.returncode != 0: |
| 187 | 518 | return [] |
| 188 | - return [s for s in r.stdout.strip().split("\n") if s] | |
| 519 | + return [s for s in r.stdout.decode("utf-8", errors="replace").strip().split("\n") if s] | |
| 189 | 520 | except (subprocess.TimeoutExpired, FileNotFoundError, OSError): |
| 190 | 521 | return [] |
| 191 | 522 | |
| @@ -199,8 +530,12 @@ | ||
| 199 | 530 | them reviewed — otherwise unreviewed commits get permanently silenced. |
| 200 | 531 | """ |
| 201 | 532 | try: |
| 533 | + # GIT_CMD globally passes core.quotePath=false (see definition) so | |
| 534 | + # non-ASCII paths in `diff --git a/... b/...` headers come through as | |
| 535 | + # raw UTF-8, not C-quoted. Required by the downstream | |
| 536 | + # parse_diff_into_files / extract_file_paths_from_diff regex. | |
| 202 | 537 | r = subprocess.run( |
| 203 | - [*GIT_CMD, "diff", "-p", "--no-color", "--no-ext-diff", base, head], | |
| 538 | + [*GIT_CMD, "diff", "-p", "--no-color", "--no-ext-diff", "--no-textconv", base, head], | |
| 204 | 539 | cwd=repo_root, capture_output=True, timeout=30, |
| 205 | 540 | ) |
| 206 | 541 | if r.returncode != 0: |
| @@ -213,9 +548,11 @@ | ||
| 213 | 548 | def _detect_main_branch(repo_root): |
| 214 | 549 | for ref in ("origin/HEAD", "origin/main", "origin/master", "main", "master"): |
| 215 | 550 | try: |
| 551 | + # See #2099: stdout is a SHA but stderr can carry non-ASCII git | |
| 552 | + # warnings — keep bytes raw to avoid cp1252 reader-thread crash. | |
| 216 | 553 | r = subprocess.run( |
| 217 | 554 | [*GIT_CMD, "rev-parse", "--verify", "-q", ref], |
| 218 | - cwd=repo_root, capture_output=True, text=True, timeout=5, | |
| 555 | + cwd=repo_root, capture_output=True, timeout=5, | |
| 219 | 556 | ) |
| 220 | 557 | if r.returncode == 0 and r.stdout.strip(): |
| 221 | 558 | return ref |
| @@ -259,19 +596,29 @@ | ||
| 259 | 596 | # %gs (the reflog subject) is `commit: <commit-msg first line>` and can |
| 260 | 597 | # contain `|`; put it LAST so split("|", 2) leaves it intact. %H is |
| 261 | 598 | # hex and %ct is integer, so the first two fields are delimiter-safe. |
| 599 | + # | |
| 600 | + # Bytes + decode utf-8/replace: %gs embeds commit-message subjects | |
| 601 | + # which git stores as raw bytes — commits can be authored in | |
| 602 | + # latin-1 / cp1252 / shift-jis etc., and text=True would raise | |
| 603 | + # UnicodeDecodeError in the subprocess reader thread on Windows | |
| 604 | + # cp1252 (subprocess.run returns r.stdout=None, then | |
| 605 | + # r.stdout.splitlines() AttributeErrors). Mirrors the existing | |
| 606 | + # migration at security_reminder_hook.py:540 — same pattern was | |
| 607 | + # missed here. See anthropics/claude-plugins-official#2056. | |
| 262 | 608 | r = subprocess.run( |
| 263 | 609 | [*GIT_CMD, "log", "-g", "-n", str(max_n), |
| 264 | 610 | "--format=%H|%ct|%gs", "HEAD"], |
| 265 | - cwd=repo_root, capture_output=True, text=True, timeout=5, | |
| 611 | + cwd=repo_root, capture_output=True, timeout=5, | |
| 266 | 612 | ) |
| 267 | - except (subprocess.TimeoutExpired, FileNotFoundError, OSError): | |
| 613 | + except (subprocess.TimeoutExpired, FileNotFoundError, OSError, ValueError): | |
| 268 | 614 | return [], 0 |
| 269 | 615 | if r.returncode != 0: |
| 270 | 616 | return [], 0 |
| 617 | + stdout = (r.stdout or b"").decode("utf-8", errors="replace") | |
| 271 | 618 | import time as _time |
| 272 | 619 | now = int(_time.time()) |
| 273 | 620 | fresh, stale = [], 0 |
| 274 | - for idx, line in enumerate(r.stdout.splitlines()): | |
| 621 | + for idx, line in enumerate(stdout.splitlines()): | |
| 275 | 622 | parts = line.split("|", 2) |
| 276 | 623 | if len(parts) != 3: |
| 277 | 624 | continue |
| @@ -305,24 +652,39 @@ | ||
| 305 | 652 | or None if git failed (unresolvable ref, not a repo, timeout). Callers |
| 306 | 653 | must distinguish None (error → don't trust as a filter) from set() |
| 307 | 654 | (genuinely nothing changed). `-c core.quotePath=false -z` keeps non-ASCII |
| 308 | - and space-containing paths intact.""" | |
| 655 | + and space-containing paths intact. | |
| 656 | + | |
| 657 | + Against the work tree the set can also hold a file whose stat changed and | |
| 658 | + whose content did not: the diff does not refresh the index. Callers | |
| 659 | + intersect with `git status`, which compares content.""" | |
| 660 | + # Decode stdout/stderr as UTF-8 with errors="replace" instead of using | |
| 661 | + # text=True. core.quotePath=false makes git emit raw UTF-8 for non-ASCII | |
| 662 | + # paths, and text=True on Windows decodes via cp1252 strict — a non-ASCII | |
| 663 | + # changed path would crash the subprocess reader thread, leave | |
| 664 | + # result.stdout=None, and propagate AttributeError out of the helper. | |
| 665 | + # Same fix shape as diffstate._list_untracked. See #2056. | |
| 309 | 666 | def _run(env): |
| 667 | + # core.quotePath=false comes from GIT_CMD globally (see definition). | |
| 310 | 668 | result = subprocess.run( |
| 311 | - [*GIT_CMD, "-c", "core.quotePath=false", "diff", "--name-only", "-z", base], | |
| 312 | - cwd=cwd, capture_output=True, text=True, timeout=30, | |
| 669 | + [*GIT_CMD, "diff", "--name-only", "-z", base], | |
| 670 | + cwd=cwd, capture_output=True, timeout=30, | |
| 313 | 671 | env=env, |
| 314 | 672 | ) |
| 315 | 673 | if result.returncode != 0: |
| 316 | - debug_log(f"_git_name_only({base!r}) rc={result.returncode}: {result.stderr[:200]}") | |
| 674 | + stderr_str = (result.stderr or b"").decode("utf-8", errors="replace") | |
| 675 | + debug_log(f"_git_name_only({base!r}) rc={result.returncode}: {stderr_str[:200]}") | |
| 317 | 676 | return None |
| 318 | - return {p for p in result.stdout.split("\0") if p} | |
| 677 | + stdout = (result.stdout or b"").decode("utf-8", errors="replace") | |
| 678 | + return {p for p in stdout.split("\0") if p} | |
| 319 | 679 | |
| 320 | 680 | try: |
| 681 | + # The diff only reads the index (diff.autoRefreshIndex is off in | |
| 682 | + # GIT_CMD); a copy is for `add -N` alone. | |
| 321 | 683 | if not include_untracked: |
| 322 | 684 | return _run(None) |
| 323 | 685 | with _temp_index(cwd) as env: |
| 324 | 686 | return _run(env) |
| 325 | - except (subprocess.TimeoutExpired, FileNotFoundError, OSError) as e: | |
| 687 | + except (subprocess.TimeoutExpired, FileNotFoundError, OSError, ValueError) as e: | |
| 326 | 688 | debug_log(f"_git_name_only({base!r}) error: {e}") |
| 327 | 689 | return None |
| 328 | 690 | |
| @@ -339,17 +701,22 @@ | ||
| 339 | 701 | collapses to `dir/`). Required so the untracked set subtracts cleanly |
| 340 | 702 | against the UPS-time `_list_untracked` snapshot, which uses ls-files and |
| 341 | 703 | therefore always lists individual files.""" |
| 704 | + # Lenient decode: same UTF-8 + errors="replace" pattern as the | |
| 705 | + # sibling helpers — a non-ASCII path in the worktree would otherwise | |
| 706 | + # crash the cp1252 reader thread on Windows. See #2056. | |
| 342 | 707 | try: |
| 708 | + # core.quotePath=false comes from GIT_CMD globally (see definition). | |
| 343 | 709 | r = subprocess.run( |
| 344 | - [*GIT_CMD, "-c", "core.quotePath=false", "status", | |
| 345 | - "--porcelain=v1", "-uall", "-z"], | |
| 346 | - cwd=cwd, capture_output=True, text=True, timeout=30, | |
| 710 | + [*GIT_CMD, "status", "--porcelain=v1", "-uall", "-z"], | |
| 711 | + cwd=cwd, capture_output=True, timeout=30, | |
| 347 | 712 | ) |
| 348 | 713 | if r.returncode != 0: |
| 349 | - debug_log(f"_git_status_porcelain rc={r.returncode}: {r.stderr[:200]}") | |
| 714 | + stderr_str = (r.stderr or b"").decode("utf-8", errors="replace") | |
| 715 | + debug_log(f"_git_status_porcelain rc={r.returncode}: {stderr_str[:200]}") | |
| 350 | 716 | return None, None |
| 351 | 717 | tracked, untracked = set(), set() |
| 352 | - entries = r.stdout.split("\0") | |
| 718 | + stdout = (r.stdout or b"").decode("utf-8", errors="replace") | |
| 719 | + entries = stdout.split("\0") | |
| 353 | 720 | i = 0 |
| 354 | 721 | while i < len(entries): |
| 355 | 722 | e = entries[i] |
| @@ -368,7 +735,9 @@ | ||
| 368 | 735 | i += 1 |
| 369 | 736 | i += 1 |
| 370 | 737 | return tracked, untracked |
| 371 | - except (subprocess.TimeoutExpired, FileNotFoundError, OSError) as e: | |
| 738 | + except (subprocess.TimeoutExpired, FileNotFoundError, OSError, ValueError) as e: | |
| 739 | + # ValueError guards against any future strict-decode regression | |
| 740 | + # so the helper degrades to (None, None) instead of crashing. | |
| 372 | 741 | debug_log(f"_git_status_porcelain error: {e}") |
| 373 | 742 | return None, None |
| 374 | 743 | |
| @@ -378,9 +747,12 @@ | ||
| 378 | 747 | """True if `maybe_ancestor` is reachable from `descendant` (i.e. HEAD |
| 379 | 748 | moved forward via commit/merge, not sideways via checkout).""" |
| 380 | 749 | try: |
| 750 | + # See #2099: only returncode matters, but text=True spawns reader | |
| 751 | + # threads that decode stderr — git error messages can carry non-ASCII | |
| 752 | + # filenames. Drop text=True to keep bytes raw, avoid cp1252 crash. | |
| 381 | 753 | result = subprocess.run( |
| 382 | 754 | [*GIT_CMD, "merge-base", "--is-ancestor", maybe_ancestor, descendant], |
| 383 | - cwd=cwd, capture_output=True, text=True, timeout=5, | |
| 755 | + cwd=cwd, capture_output=True, timeout=5, | |
| 384 | 756 | ) |
| 385 | 757 | return result.returncode == 0 |
| 386 | 758 | except (subprocess.TimeoutExpired, FileNotFoundError, OSError): |
| @@ -411,12 +783,18 @@ | ||
| 411 | 783 | # change exists to fix. |
| 412 | 784 | return "" |
| 413 | 785 | |
| 414 | - cmd = [*GIT_CMD, "diff", "--no-color", "--no-ext-diff", baseline_sha] + (["--unified=99999"] if full_context else []) + pathspec | |
| 786 | + # core.quotePath=false comes from GIT_CMD globally (see definition). | |
| 787 | + cmd = [*GIT_CMD, "diff", "--no-color", "--no-ext-diff", "--no-textconv", baseline_sha] + (["--unified=99999"] if full_context else []) + pathspec | |
| 415 | 788 | try: |
| 416 | - with _temp_index(cwd, untracked_paths) as env: | |
| 417 | - # env is None when no index could be found (bare repo / not a | |
| 418 | - # repo) — diff still runs, just without untracked-file support. | |
| 419 | - result = subprocess.run(cmd, cwd=cwd, capture_output=True, timeout=30, env=env) | |
| 789 | + if untracked_paths is not None and not untracked_paths: | |
| 790 | + # Nothing to `add -N`, and the diff only reads the index. | |
| 791 | + result = subprocess.run(cmd, cwd=cwd, capture_output=True, timeout=30) | |
| 792 | + else: | |
| 793 | + with _temp_index(cwd, untracked_paths) as env: | |
| 794 | + # env is None when there is no index (bare repo / not a repo) | |
| 795 | + # or no copy slot — diff still runs, just without | |
| 796 | + # untracked-file support. | |
| 797 | + result = subprocess.run(cmd, cwd=cwd, capture_output=True, timeout=30, env=env) | |
| 420 | 798 | if result.returncode != 0: |
| 421 | 799 | debug_log(f"git diff failed: {result.stderr[:200].decode('utf-8', errors='replace')}") |
| 422 | 800 | return None |
| @@ -45,10 +45,50 @@ | ||
| 45 | 45 | { |
| 46 | 46 | "type": "command", |
| 47 | 47 | "command": "bash \"${CLAUDE_PLUGIN_ROOT}/hooks/sg-python.sh\" \"${CLAUDE_PLUGIN_ROOT}/hooks/security_reminder_hook.py\"", |
| 48 | + "if": "Bash(git -C * commit *)", | |
| 49 | + "asyncRewake": true, | |
| 50 | + "rewakeMessage": "Background security review of commit — address or acknowledge the findings below, then continue with the user's original request or continue waiting for their reply:", | |
| 51 | + "rewakeSummary": "Commit security review found issues" | |
| 52 | + }, | |
| 53 | + { | |
| 54 | + "type": "command", | |
| 55 | + "command": "bash \"${CLAUDE_PLUGIN_ROOT}/hooks/sg-python.sh\" \"${CLAUDE_PLUGIN_ROOT}/hooks/security_reminder_hook.py\"", | |
| 48 | 56 | "if": "Bash(git push:*)", |
| 49 | 57 | "asyncRewake": true, |
| 50 | 58 | "rewakeMessage": "Background security review of pushed commits not yet reviewed — address or acknowledge the findings below, then continue with the user's original request or continue waiting for their reply:", |
| 51 | 59 | "rewakeSummary": "Push security review found issues" |
| 60 | + }, | |
| 61 | + { | |
| 62 | + "type": "command", | |
| 63 | + "command": "bash \"${CLAUDE_PLUGIN_ROOT}/hooks/sg-python.sh\" \"${CLAUDE_PLUGIN_ROOT}/hooks/security_reminder_hook.py\"", | |
| 64 | + "if": "Bash(git -C * push*)", | |
| 65 | + "asyncRewake": true, | |
| 66 | + "rewakeMessage": "Background security review of pushed commits not yet reviewed — address or acknowledge the findings below, then continue with the user's original request or continue waiting for their reply:", | |
| 67 | + "rewakeSummary": "Push security review found issues" | |
| 68 | + }, | |
| 69 | + { | |
| 70 | + "type": "command", | |
| 71 | + "command": "bash \"${CLAUDE_PLUGIN_ROOT}/hooks/sg-python.sh\" \"${CLAUDE_PLUGIN_ROOT}/hooks/security_reminder_hook.py\"", | |
| 72 | + "if": "Bash(gt create:*)", | |
| 73 | + "asyncRewake": true, | |
| 74 | + "rewakeMessage": "Background security review of commit — address or acknowledge the findings below, then continue with the user's original request or continue waiting for their reply:", | |
| 75 | + "rewakeSummary": "Commit security review found issues" | |
| 76 | + }, | |
| 77 | + { | |
| 78 | + "type": "command", | |
| 79 | + "command": "bash \"${CLAUDE_PLUGIN_ROOT}/hooks/sg-python.sh\" \"${CLAUDE_PLUGIN_ROOT}/hooks/security_reminder_hook.py\"", | |
| 80 | + "if": "Bash(gt modify:*)", | |
| 81 | + "asyncRewake": true, | |
| 82 | + "rewakeMessage": "Background security review of commit — address or acknowledge the findings below, then continue with the user's original request or continue waiting for their reply:", | |
| 83 | + "rewakeSummary": "Commit security review found issues" | |
| 84 | + }, | |
| 85 | + { | |
| 86 | + "type": "command", | |
| 87 | + "command": "bash \"${CLAUDE_PLUGIN_ROOT}/hooks/sg-python.sh\" \"${CLAUDE_PLUGIN_ROOT}/hooks/security_reminder_hook.py\"", | |
| 88 | + "if": "Bash(gt submit:*)", | |
| 89 | + "asyncRewake": true, | |
| 90 | + "rewakeMessage": "Background security review of pushed commits not yet reviewed — address or acknowledge the findings below, then continue with the user's original request or continue waiting for their reply:", | |
| 91 | + "rewakeSummary": "Push security review found issues" | |
| 52 | 92 | } |
| 53 | 93 | ], |
| 54 | 94 | "matcher": "Bash" |
| @@ -18,19 +18,90 @@ | ||
| 18 | 18 | ``from``-import) so they observe reassignment. |
| 19 | 19 | """ |
| 20 | 20 | import glob |
| 21 | +import hashlib | |
| 21 | 22 | import json |
| 22 | 23 | import os |
| 23 | 24 | import re |
| 24 | 25 | import sys |
| 26 | +import tempfile | |
| 27 | +import time | |
| 25 | 28 | import urllib.request |
| 26 | 29 | from typing import Optional, Tuple, Dict, Any, List |
| 27 | 30 | |
| 28 | 31 | import extensibility |
| 29 | 32 | import review_api |
| 30 | -from _base import debug_log, _record_usage, _PV, PROVENANCE_TAG # noqa: F401 | |
| 33 | +from _base import debug_log, _record_usage, _record_http_error, _PV, PROVENANCE_TAG, state_dir as _resolve_state_dir # noqa: F401 | |
| 31 | 34 | from session_state import with_locked_state |
| 35 | +from gitutil import git_config_env, SAFE_GIT_CONFIG | |
| 32 | 36 | |
| 33 | 37 | |
| 38 | +def _inject_agent_sdk_venv_into_syspath(state_dir): | |
| 39 | + """Prepend the agent-SDK venv's site-packages to sys.path so the SDK | |
| 40 | + import below picks it up when the user's system Python doesn't have it. | |
| 41 | + | |
| 42 | + Called from two fallback sites (3P SDK + agentic_review); shared here so | |
| 43 | + Windows pywin32 handling stays in one place. | |
| 44 | + | |
| 45 | + Returns True if any path was added. | |
| 46 | + | |
| 47 | + POSIX venv layout: `agent-sdk-venv/lib/pythonX.Y/site-packages` | |
| 48 | + Windows venv layout: `agent-sdk-venv/Lib/site-packages` (capital L, no | |
| 49 | + pythonX.Y subdir). The SDK transitively imports pywin32 on Windows, and | |
| 50 | + pywin32's `.pth` files (which add `win32/`, `win32/lib/` to sys.path and | |
| 51 | + register the DLL search dir via `pywin32_bootstrap.py`) are processed | |
| 52 | + ONLY by Python's `site.py` at interpreter startup — not when we manually | |
| 53 | + insert a path here. Without the bootstrap, the SDK's | |
| 54 | + `mcp.client.stdio → mcp.os.win32.utilities → pywintypes` import chain | |
| 55 | + fails with `ModuleNotFoundError: pywintypes` and the agentic reviewer | |
| 56 | + falls back to single-shot silently. Replicate what site.py would do. | |
| 57 | + """ | |
| 58 | + venv_root = os.path.join(state_dir, "agent-sdk-venv") | |
| 59 | + candidates = ( | |
| 60 | + glob.glob(os.path.join(venv_root, "lib", "python*", "site-packages")) | |
| 61 | + + glob.glob(os.path.join(venv_root, "Lib", "site-packages")) | |
| 62 | + # `pip install --target` fallback (ensure_agent_sdk BUILT_TARGET, used | |
| 63 | + # when venv can't bootstrap pip): a FLAT layout — packages sit directly | |
| 64 | + # in agent-sdk-libs/, not under a site-packages subdir. See #2154 | |
| 65 | + # follow-up. The pywin32 .pth bootstrap below applies here too (target | |
| 66 | + # installs don't process .pth at runtime, same as a manual venv insert). | |
| 67 | + + [os.path.join(state_dir, "agent-sdk-libs")] | |
| 68 | + ) | |
| 69 | + added = False | |
| 70 | + for sp in candidates: | |
| 71 | + if not os.path.isdir(sp) or sp in sys.path: | |
| 72 | + continue | |
| 73 | + sys.path.insert(0, sp) | |
| 74 | + added = True | |
| 75 | + if sys.platform == "win32": | |
| 76 | + _bootstrap_pywin32(sp) | |
| 77 | + return added | |
| 78 | + | |
| 79 | + | |
| 80 | +def _bootstrap_pywin32(site_packages_dir): | |
| 81 | + """Manually replicate the pywin32 `.pth` bootstrap so a venv added via | |
| 82 | + `sys.path.insert()` (not site.py) can still import `pywintypes`. No-op | |
| 83 | + when the venv doesn't include pywin32. Failures are swallowed — the | |
| 84 | + SDK import below will raise its own ImportError and the caller's | |
| 85 | + fallback path handles it cleanly.""" | |
| 86 | + try: | |
| 87 | + win32 = os.path.join(site_packages_dir, "win32") | |
| 88 | + win32_lib = os.path.join(win32, "lib") | |
| 89 | + for d in (win32, win32_lib): | |
| 90 | + if os.path.isdir(d) and d not in sys.path: | |
| 91 | + sys.path.insert(0, d) | |
| 92 | + bootstrap = os.path.join(win32_lib, "pywin32_bootstrap.py") | |
| 93 | + if os.path.isfile(bootstrap): | |
| 94 | + import importlib.util | |
| 95 | + spec = importlib.util.spec_from_file_location( | |
| 96 | + "pywin32_bootstrap", bootstrap, | |
| 97 | + ) | |
| 98 | + if spec and spec.loader: | |
| 99 | + mod = importlib.util.module_from_spec(spec) | |
| 100 | + spec.loader.exec_module(mod) | |
| 101 | + except Exception as e: | |
| 102 | + debug_log(f"pywin32 bootstrap failed (may break SDK import on Windows): {e}") | |
| 103 | + | |
| 104 | + | |
| 34 | 105 | # Plan Security Check Configuration |
| 35 | 106 | ANTHROPIC_API_KEY = os.environ.get("ANTHROPIC_API_KEY", "") |
| 36 | 107 | # OAuth access token — Claude Code passes this for /login users. |
| @@ -57,11 +128,19 @@ | ||
| 57 | 128 | ANTHROPIC_API_KEY or ANTHROPIC_AUTH_TOKEN or _HAS_3P_PROVIDER_AT_LOAD |
| 58 | 129 | ) |
| 59 | 130 | |
| 60 | -# Model for security review. Default chosen for its precision profile on | |
| 131 | +# Model for security review. Opus is the default for its precision profile on | |
| 61 | 132 | # interruptive review surfaces — false positives are the dominant uninstall |
| 62 | 133 | # driver, so the default favors precision over recall and over latency. |
| 63 | 134 | # Override via the SECURITY_REVIEW_MODEL env var (see README). |
| 64 | -SECURITY_REVIEW_MODEL = os.environ.get("SECURITY_REVIEW_MODEL", "").strip() or "claude-opus-4-7" | |
| 135 | +# | |
| 136 | +# Without an override the review follows the newest Opus rather than a | |
| 137 | +# release named here: default_review_model() resolves it per call path. | |
| 138 | +# _PINNED_REVIEW_MODEL is only what a review uses when that lookup fails. | |
| 139 | +_PINNED_REVIEW_MODEL = "claude-opus-5-5" | |
| 140 | +# Claude Code's own alias. The CLI the Agent SDK spawns resolves it to the | |
| 141 | +# newest Opus it knows how to drive, in the provider's id form on 3P. | |
| 142 | +_CLI_OPUS_ALIAS = "opus" | |
| 143 | +SECURITY_REVIEW_MODEL = os.environ.get("SECURITY_REVIEW_MODEL", "").strip() or _PINNED_REVIEW_MODEL | |
| 65 | 144 | |
| 66 | 145 | # OAuth subscriber tokens (ANTHROPIC_AUTH_TOKEN) require this exact system prompt |
| 67 | 146 | # for api.anthropic.com/v1/messages — the API checks for one of the known Claude |
| @@ -188,8 +267,8 @@ | ||
| 188 | 267 | _auth_prefer_token = False |
| 189 | 268 | |
| 190 | 269 | |
| 191 | -def _build_auth_headers(use_token): | |
| 192 | - betas = ["structured-outputs-2025-11-13"] | |
| 270 | +def _build_auth_headers(use_token, betas=("structured-outputs-2025-11-13",)): | |
| 271 | + betas = list(betas) | |
| 193 | 272 | headers = { |
| 194 | 273 | "Content-Type": "application/json", |
| 195 | 274 | "anthropic-version": "2023-06-01", |
| @@ -199,7 +278,8 @@ | ||
| 199 | 278 | betas.append("oauth-2025-04-20") |
| 200 | 279 | else: |
| 201 | 280 | headers["x-api-key"] = ANTHROPIC_API_KEY |
| 202 | - headers["anthropic-beta"] = ",".join(betas) | |
| 281 | + if betas: | |
| 282 | + headers["anthropic-beta"] = ",".join(betas) | |
| 203 | 283 | return headers |
| 204 | 284 | |
| 205 | 285 | |
| @@ -210,7 +290,10 @@ | ||
| 210 | 290 | _ADAPTIVE_THINKING_MODELS = ( |
| 211 | 291 | "claude-opus-4-6", |
| 212 | 292 | "claude-opus-4-7", |
| 293 | + "claude-opus-4-8", | |
| 294 | + "claude-opus-5", | |
| 213 | 295 | "claude-sonnet-4-6", |
| 296 | + "claude-sonnet-5", | |
| 214 | 297 | ) |
| 215 | 298 | _LEGACY_THINKING_MODELS = ( |
| 216 | 299 | "claude-3-", |
| @@ -271,6 +354,159 @@ | ||
| 271 | 354 | return False |
| 272 | 355 | |
| 273 | 356 | |
| 357 | +# ── Default review model ───────────────────────────────────────────────── | |
| 358 | +# The API has no alias for "newest Opus", so the raw-HTTP path asks the | |
| 359 | +# endpoint it is about to call (GET /v1/models) and keeps the answer in the | |
| 360 | +# state dir. Asking that endpoint, not api.anthropic.com, means a gateway | |
| 361 | +# that serves an older Opus gets a model it can actually route. | |
| 362 | + | |
| 363 | +_REVIEW_MODEL_CACHE_FILE = "review_model.json" | |
| 364 | +_REVIEW_MODEL_TTL_SECONDS = 24 * 3600 | |
| 365 | +# A failed lookup is remembered too, for less long: otherwise an endpoint | |
| 366 | +# without /v1/models costs every hook fire the lookup timeout. | |
| 367 | +_REVIEW_MODEL_RETRY_SECONDS = 3600 | |
| 368 | +_REVIEW_MODEL_LOOKUP_TIMEOUT = 5 | |
| 369 | +_REVIEW_MODEL_LISTING_MAX_BYTES = 4 * 1024 * 1024 | |
| 370 | + | |
| 371 | +# claude-opus-<major>[-<minor>][-<yyyymmdd>]. Anything else a listing or the | |
| 372 | +# cache file holds (other families, gateway-prefixed names, free text) is | |
| 373 | +# never sent as a model id. | |
| 374 | +_OPUS_ID = re.compile(r"claude-opus-(\d{1,2})(?:-(\d{1,2}))?(?:-(\d{8}))?") | |
| 375 | + | |
| 376 | +_review_model_memo: Optional[str] = None | |
| 377 | + | |
| 378 | + | |
| 379 | +def _newest_opus(model_ids) -> Optional[str]: | |
| 380 | + """The highest-versioned Opus id in `model_ids`, or None.""" | |
| 381 | + best = None | |
| 382 | + for model_id in model_ids: | |
| 383 | + m = _OPUS_ID.fullmatch(model_id) if isinstance(model_id, str) else None | |
| 384 | + if not m: | |
| 385 | + continue | |
| 386 | + version = (int(m.group(1)), int(m.group(2) or 0), m.group(3) or "") | |
| 387 | + if best is None or version > best[0]: | |
| 388 | + best = (version, model_id) | |
| 389 | + return best[1] if best else None | |
| 390 | + | |
| 391 | + | |
| 392 | +def _fetch_newest_opus() -> Optional[str]: | |
| 393 | + """Newest Opus the configured endpoint lists; None if it can't say.""" | |
| 394 | + # Newest first, so one modest page is enough; an org with many models | |
| 395 | + # otherwise returns a megabyte-sized listing. | |
| 396 | + url = _anthropic_base_url() + "/v1/models?limit=100" | |
| 397 | + use_token = _auth_prefer_token or not ANTHROPIC_API_KEY | |
| 398 | + for _ in range(2): | |
| 399 | + request = urllib.request.Request( | |
| 400 | + url, headers=_build_auth_headers(use_token, betas=()), method="GET", | |
| 401 | + ) | |
| 402 | + try: | |
| 403 | + with urllib.request.urlopen( | |
| 404 | + request, timeout=_REVIEW_MODEL_LOOKUP_TIMEOUT, | |
| 405 | + ) as response: | |
| 406 | + listing = json.loads( | |
| 407 | + response.read(_REVIEW_MODEL_LISTING_MAX_BYTES).decode("utf-8") | |
| 408 | + ) | |
| 409 | + entries = listing.get("data") if isinstance(listing, dict) else None | |
| 410 | + if not isinstance(entries, list): | |
| 411 | + debug_log("review model lookup: no model list in response") | |
| 412 | + return None | |
| 413 | + ids = [e.get("id") for e in entries if isinstance(e, dict)] | |
| 414 | + if listing.get("has_more"): | |
| 415 | + # A truncated page may hold no current Opus at all (an org's | |
| 416 | + # own models can fill it); never let it pick an older one. | |
| 417 | + ids.append(_PINNED_REVIEW_MODEL) | |
| 418 | + return _newest_opus(ids) | |
| 419 | + except urllib.error.HTTPError as e: | |
| 420 | + if e.code == 401 and not use_token and ANTHROPIC_AUTH_TOKEN: | |
| 421 | + use_token = True | |
| 422 | + continue | |
| 423 | + debug_log(f"review model lookup: HTTP {e.code}") | |
| 424 | + return None | |
| 425 | + except Exception as e: | |
| 426 | + debug_log(f"review model lookup failed: {type(e).__name__}") | |
| 427 | + return None | |
| 428 | + return None | |
| 429 | + | |
| 430 | + | |
| 431 | +def _read_review_model_cache(path, endpoint_key, now): | |
| 432 | + """(hit, model) from the cache file. `model` is None for a remembered | |
| 433 | + failed lookup. The file is user-writable state: every field is checked | |
| 434 | + and a model id is accepted only if it has the shape of an Opus id.""" | |
| 435 | + try: | |
| 436 | + with open(path, "r", encoding="utf-8") as f: | |
| 437 | + entry = json.loads(f.read(4096)) | |
| 438 | + except (OSError, ValueError): | |
| 439 | + return False, None | |
| 440 | + if not isinstance(entry, dict) or entry.get("endpoint") != endpoint_key: | |
| 441 | + return False, None | |
| 442 | + at = entry.get("at") | |
| 443 | + if isinstance(at, bool) or not isinstance(at, (int, float)): | |
| 444 | + return False, None | |
| 445 | + model = entry.get("model") | |
| 446 | + if model is None: | |
| 447 | + ttl = _REVIEW_MODEL_RETRY_SECONDS | |
| 448 | + elif isinstance(model, str) and _OPUS_ID.fullmatch(model): | |
| 449 | + ttl = _REVIEW_MODEL_TTL_SECONDS | |
| 450 | + else: | |
| 451 | + return False, None | |
| 452 | + if not 0 <= now - at < ttl: | |
| 453 | + return False, None | |
| 454 | + return True, model | |
| 455 | + | |
| 456 | + | |
| 457 | +def _write_review_model_cache(path, endpoint_key, model, now): | |
| 458 | + tmp = None | |
| 459 | + try: | |
| 460 | + os.makedirs(os.path.dirname(path), exist_ok=True) | |
| 461 | + fd, tmp = tempfile.mkstemp(dir=os.path.dirname(path), prefix=".review_model.") | |
| 462 | + with os.fdopen(fd, "w", encoding="utf-8") as f: | |
| 463 | + json.dump({"endpoint": endpoint_key, "model": model, "at": now}, f) | |
| 464 | + os.replace(tmp, path) | |
| 465 | + except OSError as e: | |
| 466 | + debug_log(f"review model cache not written: {type(e).__name__}") | |
| 467 | + if tmp: | |
| 468 | + try: | |
| 469 | + os.unlink(tmp) | |
| 470 | + except OSError: | |
| 471 | + pass | |
| 472 | + | |
| 473 | + | |
| 474 | +def default_review_model() -> str: | |
| 475 | + """Model for a review the user hasn't pinned with SECURITY_REVIEW_MODEL. | |
| 476 | + | |
| 477 | + 3P providers go through the Agent SDK, so the CLI alias does the work. | |
| 478 | + 1P and gateways get the newest Opus the endpoint lists, looked up at most | |
| 479 | + once per _REVIEW_MODEL_TTL_SECONDS; _PINNED_REVIEW_MODEL when the lookup | |
| 480 | + can't answer. Never raises. | |
| 481 | + """ | |
| 482 | + global _review_model_memo | |
| 483 | + explicit = os.environ.get("SECURITY_REVIEW_MODEL", "").strip() | |
| 484 | + if explicit: | |
| 485 | + return explicit | |
| 486 | + if _is_3p_provider(): | |
| 487 | + return _CLI_OPUS_ALIAS | |
| 488 | + if not HAS_API_CREDENTIALS: | |
| 489 | + return _PINNED_REVIEW_MODEL | |
| 490 | + if _review_model_memo: | |
| 491 | + return _review_model_memo | |
| 492 | + try: | |
| 493 | + path = os.path.join(_resolve_state_dir(), _REVIEW_MODEL_CACHE_FILE) | |
| 494 | + # Hashed: a gateway URL can carry credentials. | |
| 495 | + endpoint_key = hashlib.sha256( | |
| 496 | + _anthropic_base_url().encode("utf-8") | |
| 497 | + ).hexdigest()[:16] | |
| 498 | + now = time.time() | |
| 499 | + hit, model = _read_review_model_cache(path, endpoint_key, now) | |
| 500 | + if not hit: | |
| 501 | + model = _fetch_newest_opus() | |
| 502 | + _write_review_model_cache(path, endpoint_key, model, now) | |
| 503 | + except Exception as e: | |
| 504 | + debug_log(f"review model resolution failed: {type(e).__name__}") | |
| 505 | + model = None | |
| 506 | + _review_model_memo = model or _PINNED_REVIEW_MODEL | |
| 507 | + return _review_model_memo | |
| 508 | + | |
| 509 | + | |
| 274 | 510 | def _call_claude_via_sdk(prompt, output_schema, *, max_tokens=16000, model=None): |
| 275 | 511 | """Single-turn SDK call as a substitute for the HTTP _call_claude path on |
| 276 | 512 | 3P providers. Uses the same `output_format` JSON-schema contract so the |
| @@ -294,16 +530,8 @@ | ||
| 294 | 530 | # Try the venv ensure_agent_sdk.py builds. Same fallback logic as |
| 295 | 531 | # agentic_review() — duplicated here so the 3P path doesn't require |
| 296 | 532 | # the agentic path to have run first. |
| 297 | - _state_dir = os.environ.get( | |
| 298 | - "SECURITY_WARNINGS_STATE_DIR", | |
| 299 | - os.path.expanduser("~/.claude/security"), | |
| 300 | - ) | |
| 301 | - for _sp in glob.glob( | |
| 302 | - os.path.join(_state_dir, "agent-sdk-venv", "lib", | |
| 303 | - "python*", "site-packages") | |
| 304 | - ): | |
| 305 | - if os.path.isdir(_sp) and _sp not in sys.path: | |
| 306 | - sys.path.insert(0, _sp) | |
| 533 | + _state_dir = _resolve_state_dir() | |
| 534 | + _inject_agent_sdk_venv_into_syspath(_state_dir) | |
| 307 | 535 | try: |
| 308 | 536 | import asyncio as _asyncio # noqa: F811 |
| 309 | 537 | from claude_agent_sdk import ( # noqa: F401,F811 |
| @@ -315,10 +543,12 @@ | ||
| 315 | 543 | except Exception as e: |
| 316 | 544 | debug_log(f"3P sdk-single-turn: SDK unavailable ({e})") |
| 317 | 545 | _last_call_claude_http_error = -1 |
| 546 | + _record_http_error(-1) | |
| 318 | 547 | return None |
| 319 | 548 | |
| 320 | 549 | cli_path = os.environ.get("SG_AGENTIC_CLI_PATH") or None |
| 321 | - chosen_model = model or SECURITY_REVIEW_MODEL | |
| 550 | + default_model = default_review_model() | |
| 551 | + chosen_model = model or default_model | |
| 322 | 552 | |
| 323 | 553 | # Capture child claude stderr so a failing 3P call surfaces the real |
| 324 | 554 | # error (auth missing, model id wrong, etc.) in the debug log instead |
| @@ -335,10 +565,10 @@ | ||
| 335 | 565 | model=chosen_model, |
| 336 | 566 | output_format={"type": "json_schema", "schema": output_schema}, |
| 337 | 567 | # Identical --model/--fallback-model is rejected by the CLI at |
| 338 | - # startup; chosen_model defaults to SECURITY_REVIEW_MODEL, so | |
| 339 | - # only pass a fallback when it actually differs. | |
| 568 | + # startup; chosen_model defaults to default_model, so only pass | |
| 569 | + # a fallback when it actually differs. | |
| 340 | 570 | fallback_model=( |
| 341 | - SECURITY_REVIEW_MODEL if chosen_model != SECURITY_REVIEW_MODEL else None | |
| 571 | + default_model if chosen_model != default_model else None | |
| 342 | 572 | ), |
| 343 | 573 | env=_agentic_spawn_env(), |
| 344 | 574 | stderr=lambda l: _captured_stderr.append(l), |
| @@ -372,6 +602,7 @@ | ||
| 372 | 602 | except _asyncio.TimeoutError: |
| 373 | 603 | debug_log("3P sdk-single-turn: timeout after 60s") |
| 374 | 604 | _last_call_claude_http_error = -1 |
| 605 | + _record_http_error(-1) | |
| 375 | 606 | return None |
| 376 | 607 | except Exception as e: |
| 377 | 608 | debug_log(f"3P sdk-single-turn: query failed ({e})") |
| @@ -380,14 +611,17 @@ | ||
| 380 | 611 | for _l in _captured_stderr[:20]: |
| 381 | 612 | debug_log(f" | {_l.rstrip()}") |
| 382 | 613 | _last_call_claude_http_error = -1 |
| 614 | + _record_http_error(-1) | |
| 383 | 615 | return None |
| 384 | 616 | |
| 385 | 617 | |
| 386 | 618 | def _call_claude(prompt, output_schema, thinking_budget=10000, max_tokens=16000, model=None, |
| 387 | - retry_5xx=True): | |
| 619 | + retry_5xx=True, cacheable_prefix=None): | |
| 388 | 620 | """ |
| 389 | 621 | Call the configured LLM model with extended thinking and structured outputs. |
| 390 | - Model defaults to Sonnet 4.6 but can be overridden via SECURITY_REVIEW_MODEL env var. | |
| 622 | + cacheable_prefix: static text sent ahead of `prompt` as its own content block | |
| 623 | + with cache_control, so repeated calls share a cached request prefix. | |
| 624 | + Model defaults to default_review_model(); SECURITY_REVIEW_MODEL overrides it. | |
| 391 | 625 | Returns parsed JSON response or None on failure. |
| 392 | 626 | On failure, sets module-level _last_call_claude_http_error to the HTTP status |
| 393 | 627 | (or -1 for network/timeout) so callers can distinguish API failure from an |
| @@ -408,6 +642,8 @@ | ||
| 408 | 642 | # child claude CLI. Note: thinking_budget/retry_5xx don't pass |
| 409 | 643 | # through — the SDK manages retries (529) and thinking config |
| 410 | 644 | # internally per the chosen model. |
| 645 | + if cacheable_prefix: | |
| 646 | + prompt = cacheable_prefix + "\n\n" + prompt | |
| 411 | 647 | return _call_claude_via_sdk(prompt, output_schema, |
| 412 | 648 | max_tokens=max_tokens, model=model) |
| 413 | 649 | |
| @@ -421,15 +657,35 @@ | ||
| 421 | 657 | use_token = _auth_prefer_token or not ANTHROPIC_API_KEY |
| 422 | 658 | headers = _build_auth_headers(use_token) |
| 423 | 659 | |
| 660 | + if cacheable_prefix: | |
| 661 | + content = [ | |
| 662 | + {"type": "text", "text": cacheable_prefix, | |
| 663 | + "cache_control": {"type": "ephemeral"}}, | |
| 664 | + {"type": "text", "text": prompt}, | |
| 665 | + ] | |
| 666 | + else: | |
| 667 | + content = prompt | |
| 668 | + | |
| 424 | 669 | payload = { |
| 425 | - "model": model or SECURITY_REVIEW_MODEL, | |
| 670 | + "model": model or default_review_model(), | |
| 426 | 671 | "max_tokens": max_tokens, |
| 427 | 672 | "system": CLAUDE_CODE_SYSTEM_PROMPT, |
| 428 | - "messages": [{"role": "user", "content": prompt}], | |
| 429 | - "output_format": { | |
| 430 | - "type": "json_schema", | |
| 431 | - "schema": output_schema | |
| 432 | - } | |
| 673 | + "messages": [{"role": "user", "content": content}], | |
| 674 | + # API moved the structured-output schema from top-level `output_format` | |
| 675 | + # to `output_config.format` per | |
| 676 | + # https://platform.claude.com/docs/en/build-with-claude/structured-outputs. | |
| 677 | + # The old form "continues to work for a transition period" for some | |
| 678 | + # auth modes (API key + non-streaming), but is rejected with | |
| 679 | + # `invalid_request_error: output_format: This field is deprecated. | |
| 680 | + # Use 'output_config.format' instead.` for others (OAuth Bearer + | |
| 681 | + # newer CLI versions hit it consistently — reporter saw 462 errors | |
| 682 | + # in one day). See #2098. | |
| 683 | + "output_config": { | |
| 684 | + "format": { | |
| 685 | + "type": "json_schema", | |
| 686 | + "schema": output_schema, | |
| 687 | + }, | |
| 688 | + }, | |
| 433 | 689 | } |
| 434 | 690 | if thinking_budget > 0: |
| 435 | 691 | # Models trained on adaptive thinking (4.6+) reject the budget_tokens |
| @@ -437,7 +693,10 @@ | ||
| 437 | 693 | # models (4.5 and earlier, all 3.x) reject adaptive. Pick by model. |
| 438 | 694 | if _model_supports_adaptive_thinking(payload["model"]): |
| 439 | 695 | payload["thinking"] = {"type": "adaptive"} |
| 440 | - payload["output_config"] = {"effort": "high"} | |
| 696 | + # Merge `effort` into the existing output_config dict (which | |
| 697 | + # now carries the `format` schema) rather than reassigning — | |
| 698 | + # otherwise the schema is silently overwritten. See #2098. | |
| 699 | + payload["output_config"]["effort"] = "high" | |
| 441 | 700 | else: |
| 442 | 701 | payload["thinking"] = { |
| 443 | 702 | "type": "enabled", |
| @@ -475,6 +734,7 @@ | ||
| 475 | 734 | error_body = e.read().decode("utf-8") if e.fp else "" |
| 476 | 735 | debug_log(f"API error: {e.code} - {error_body[:200]}") |
| 477 | 736 | _last_call_claude_http_error = e.code |
| 737 | + _record_http_error(e.code) | |
| 478 | 738 | return None |
| 479 | 739 | except (urllib.error.URLError, TimeoutError) as e: |
| 480 | 740 | if attempt < 2: |
| @@ -484,6 +744,7 @@ | ||
| 484 | 744 | else: |
| 485 | 745 | debug_log(f"Request failed after retries: {e}") |
| 486 | 746 | _last_call_claude_http_error = -1 |
| 747 | + _record_http_error(-1) | |
| 487 | 748 | return None |
| 488 | 749 | |
| 489 | 750 | if not response_data: |
| @@ -492,6 +753,7 @@ | ||
| 492 | 753 | # call uses the token; record the 401 so callers don't see error=None. |
| 493 | 754 | if _last_call_claude_http_error is None: |
| 494 | 755 | _last_call_claude_http_error = 401 |
| 756 | + _record_http_error(401) | |
| 495 | 757 | return None |
| 496 | 758 | |
| 497 | 759 | # Find the text block (skip thinking blocks) |
| @@ -519,7 +781,7 @@ | ||
| 519 | 781 | |
| 520 | 782 | |
| 521 | 783 | def _call_claude_dual_or(prompt, output_schema, *, bool_key: str, list_key: str, |
| 522 | - thinking_budget=10000, max_tokens=16000): | |
| 784 | + thinking_budget=10000, max_tokens=16000, cacheable_prefix=None): | |
| 523 | 785 | """Run prompt through the model 2× in parallel and OR-merge the results. |
| 524 | 786 | |
| 525 | 787 | The second look samples the model again on the same prompt — independent |
| @@ -542,28 +804,30 @@ | ||
| 542 | 804 | from concurrent.futures import ThreadPoolExecutor |
| 543 | 805 | |
| 544 | 806 | explicit = os.environ.get("SECURITY_REVIEW_MODEL", "").strip() |
| 545 | - primary = explicit or SECURITY_REVIEW_MODEL | |
| 807 | + primary = default_review_model() | |
| 546 | 808 | |
| 547 | 809 | if not _dual_or_enabled(): |
| 548 | 810 | # Single-call path. Reuse the same sonnet-fallback retry as a dual_or |
| 549 | 811 | # leg so a 529/400 on the primary doesn't drop recall to zero. |
| 550 | 812 | r = _call_claude(prompt, output_schema, thinking_budget=thinking_budget, |
| 551 | - max_tokens=max_tokens, model=primary, retry_5xx=False) | |
| 813 | + max_tokens=max_tokens, model=primary, retry_5xx=False, | |
| 814 | + cacheable_prefix=cacheable_prefix) | |
| 552 | 815 | if r is None and not explicit: |
| 553 | 816 | debug_log(f"single: {primary} failed, falling back to sonnet") |
| 554 | 817 | r = _call_claude(prompt, output_schema, thinking_budget=thinking_budget, |
| 555 | 818 | max_tokens=max_tokens, model="claude-sonnet-4-6", |
| 556 | - retry_5xx=True) | |
| 819 | + retry_5xx=True, cacheable_prefix=cacheable_prefix) | |
| 557 | 820 | return r |
| 558 | 821 | |
| 559 | 822 | def _leg(): |
| 560 | 823 | r = _call_claude(prompt, output_schema, thinking_budget=thinking_budget, |
| 561 | - max_tokens=max_tokens, model=primary, retry_5xx=False) | |
| 824 | + max_tokens=max_tokens, model=primary, retry_5xx=False, | |
| 825 | + cacheable_prefix=cacheable_prefix) | |
| 562 | 826 | if r is None and not explicit: |
| 563 | 827 | debug_log(f"dual_or: {primary} leg failed, falling back to sonnet") |
| 564 | 828 | r = _call_claude(prompt, output_schema, thinking_budget=thinking_budget, |
| 565 | 829 | max_tokens=max_tokens, model="claude-sonnet-4-6", |
| 566 | - retry_5xx=True) | |
| 830 | + retry_5xx=True, cacheable_prefix=cacheable_prefix) | |
| 567 | 831 | return r |
| 568 | 832 | |
| 569 | 833 | with ThreadPoolExecutor(max_workers=2) as ex: |
| @@ -707,145 +971,60 @@ | ||
| 707 | 971 | return kept, len(vulns) - len(kept) |
| 708 | 972 | |
| 709 | 973 | |
| 710 | -def analyze_code_security(files: List[Tuple[str, str]], is_diff: bool = False, previous_findings: Optional[List[str]] = None) -> Tuple[Optional[str], List[Dict[str, Any]]]: | |
| 711 | - """ | |
| 712 | - Use Haiku to perform a security review of code. | |
| 713 | - files: list of (file_path, content_or_diff) tuples | |
| 714 | - is_diff: if True, the content is a unified diff rather than full file contents | |
| 715 | - previous_findings: list of category strings from earlier stop hook firings this turn, | |
| 716 | - used to prompt the reviewer to verify those issues were actually fixed. | |
| 717 | - Returns (formatted guidance string or None, list of vuln dicts with severity/category). | |
| 718 | - """ | |
| 719 | - if not HAS_API_CREDENTIALS or not files: | |
| 720 | - return None, [] | |
| 974 | +# Static review rubric, sent as its own content block with cache_control so the | |
| 975 | +# request prefix is byte-identical across calls. Keep it free of per-call text. | |
| 976 | +_CODE_REVIEW_RUBRIC = """You are a security expert reviewing code for CONCRETE security vulnerabilities that an attacker could exploit. | |
| 721 | 977 | |
| 722 | - # Build language context from file extensions | |
| 723 | - lang_hints = { | |
| 724 | - ".go": "Go", ".java": "Java/Spring Boot", ".py": "Python", | |
| 725 | - ".rb": "Ruby", ".php": "PHP", ".rs": "Rust", | |
| 726 | - ".ts": "TypeScript", ".js": "JavaScript", ".jsx": "JavaScript/React", | |
| 727 | - ".tsx": "TypeScript/React", ".ejs": "EJS templates", | |
| 728 | - ".html": "HTML/templates", ".properties": "Java properties", | |
| 729 | - ".yaml": "YAML config", ".yml": "YAML config", | |
| 730 | - } | |
| 731 | - languages = set() | |
| 732 | - for fp, _ in files: | |
| 733 | - ext = os.path.splitext(fp)[1].lower() | |
| 734 | - if ext in lang_hints: | |
| 735 | - languages.add(lang_hints[ext]) | |
| 736 | - language = ", ".join(sorted(languages)) if languages else "server-side" | |
| 737 | - | |
| 738 | - files = _cap_files_for_prompt(files) | |
| 739 | - | |
| 740 | - # Build the files section | |
| 741 | - files_section = [] | |
| 742 | - for fp, content in files: | |
| 743 | - ext = os.path.splitext(fp)[1].lower() | |
| 744 | - label = "DIFF" if is_diff else "FILE" | |
| 745 | - files_section.append(f"=== {label}: {fp} ===\n```{ext.lstrip('.')}\n{content}\n```") | |
| 746 | - files_text = "\n\n".join(files_section) | |
| 747 | - | |
| 748 | - content_desc = "diff" if is_diff else "code" | |
| 749 | - | |
| 750 | - if is_diff: | |
| 751 | - diff_instruction = """Note: You are reviewing a unified diff. Unmarked lines (starting with a space) are UNCHANGED context — they were already in the file before this session. Lines starting with + are ADDITIONS made in this session. Lines starting with - are REMOVALS. | |
| 752 | - | |
| 753 | -CRITICAL: ONLY flag vulnerabilities that are NEWLY INTRODUCED in + lines. Do NOT flag: | |
| 754 | -- Issues in unmarked context lines (space-prefixed = pre-existing code). Even if a context line contains SECRET_KEY = 'hardcoded', DEBUG=True, hardcoded passwords, SQL injection, or any other vulnerability — it is PRE-EXISTING and must be ignored. | |
| 755 | -- Issues where the SAME pattern existed in the removed (-) lines and was re-added in + lines (this means the code was rewritten/reformatted but the pattern is pre-existing) | |
| 756 | -- Pre-existing patterns that Claude simply preserved when rewriting a file | |
| 757 | -- Any vulnerability whose vulnerable code snippet appears in context (space-prefixed) lines | |
| 758 | -- Vulnerabilities in the ORIGINAL/STARTER code that the developer was given to work with. If a file was fully rewritten (all lines show as - then +), compare the + content against the - content. Only flag NEWLY INTRODUCED patterns that did NOT exist in the - lines. | |
| 759 | -- Issues OUTSIDE THE SCOPE of what the developer was asked to do. If the task was "add logging middleware" and the starter code has a hardcoded SECRET_KEY, that is pre-existing and out of scope — do NOT flag it. | |
| 760 | - | |
| 761 | -A vulnerability is ONLY new if the + lines introduce a pattern that did NOT exist anywhere in the - lines or context lines of the same file. | |
| 762 | - | |
| 763 | -EXCEPTION — data flow to pre-existing sinks: If + lines route user-controlled data to a PRE-EXISTING dangerous sink (like `new Function()`, `eval()`, `exec()`, or shell string interpolation in context lines), this IS a new vulnerability. The sink was already there, but the new code created a new attack path to it. Flag this as a new vulnerability in the + lines.""" | |
| 764 | - else: | |
| 765 | - diff_instruction = "" | |
| 766 | - | |
| 767 | - structured_prev = [f for f in (previous_findings or []) if isinstance(f, dict)] | |
| 768 | - if structured_prev: | |
| 769 | - prev_lines = "\n".join( | |
| 770 | - f" - {f.get('filePath', '?')} [{f.get('category', '?')}]: {f.get('vulnerableCode', '?')}" | |
| 771 | - for f in structured_prev | |
| 772 | - ) | |
| 773 | - prev_section = ( | |
| 774 | - "PREVIOUS FINDINGS (already surfaced to the developer earlier this turn — DO NOT re-flag):\n" | |
| 775 | - "The exact findings below were already shown to the developer, who has either fixed them or " | |
| 776 | - "acknowledged them as not applicable. DO NOT report any finding whose (filePath, category) pair " | |
| 777 | - "matches an entry below — it was already handled. The vulnerableCode may differ slightly from " | |
| 778 | - "what you see now (diff context lines shift between fires) — match on file + category, not exact " | |
| 779 | - "code bytes. ONLY re-flag a (filePath, category) from this list if the code at that location was " | |
| 780 | - "CHANGED since the prior review and the change is an incomplete fix or introduces a new issue.\n" | |
| 781 | - f"{prev_lines}\n" | |
| 782 | - ) | |
| 783 | - else: | |
| 784 | - prev_section = "" | |
| 785 | - | |
| 786 | - prompt = """You are a security expert reviewing {language} {content_desc}. Analyze the {content_desc} below for CONCRETE security vulnerabilities that an attacker could exploit. | |
| 787 | - | |
| 788 | -{diff_instruction} | |
| 789 | - | |
| 790 | -{prev_section} | |
| 791 | - | |
| 792 | -For each vulnerability found, provide: | |
| 793 | -1. The file path where it occurs (use the exact path from the === {file_type}: header) | |
| 794 | -2. The vulnerability category | |
| 795 | -3. The specific vulnerable code (quote the exact line(s)) | |
| 796 | -4. How an attacker would exploit it | |
| 797 | -5. A specific code fix | |
| 798 | - | |
| 799 | 978 | IMPORTANT vulnerability categories to check: |
| 800 | 979 | |
| 801 | 980 | **Command Injection**: Is user input passed to shell commands or system exec calls? In Go, exec.Command("sh", "-c", userInput) is injectable. Even exec.Command("cmd", userArg) can be dangerous if userArg isn't validated (e.g., a hostname could contain shell metacharacters in some contexts). Safe: pass each argument separately without invoking a shell, AND validate the input format. |
| 802 | 981 | |
| 803 | 982 | **Path Traversal**: Is user input used to construct file paths? Key insight: filepath.Join() in Go does NOT prevent path traversal — filepath.Join("/var/log", "../../etc/passwd") returns "/etc/passwd". Same for Python's os.path.join() and Java's Paths.get().resolve(). CRITICAL: `path.resolve()`/`filepath.Clean()`/`normalize()` are LEXICAL — they collapse `..` but do NOT dereference symlinks, so `startsWith(baseDir)` after them is symlink-bypassable. Call `fs.realpathSync()`/`os.path.realpath()`/`filepath.EvalSymlinks()` FIRST, then check the result starts with the realpath of baseDir. |
| 804 | 983 | |
| 805 | -**SQL Injection**: Is user input concatenated into SQL queries instead of using parameterized queries? This includes f-string interpolation (e.g., `f"WHERE name = '{{user_input}}'"`) and string concatenation (e.g., `"WHERE name = '" + user_input + "'"`). Even if input appears to be validated upstream, use parameterized queries. In Python: `cursor.execute('WHERE name = %s', (user_input,))`. In Go: `db.Query('WHERE name = $1', userInput)`. | |
| 984 | +**SQL Injection**: Is user input concatenated into SQL queries instead of using parameterized queries? This includes f-string interpolation (e.g., `f"WHERE name = '{user_input}'"`) and string concatenation (e.g., `"WHERE name = '" + user_input + "'"`). Even if input appears to be validated upstream, use parameterized queries. In Python: `cursor.execute('WHERE name = %s', (user_input,))`. In Go: `db.Query('WHERE name = $1', userInput)`. | |
| 806 | 985 | |
| 807 | 986 | A NEW security-gate parameter (group/role/tool/permission/scope) is safe only if (a) the gate is enforced unconditionally, OR (b) when its enabling condition is False the function raises/denies. If execution can continue past the new gate unchecked, flag fail-open — a later check may be vacuous when the new gate was the caller's only constraint. |
| 808 | 987 | |
| 809 | -**Authorization (IDOR / scoping / visibility)**: A handler that returns or modifies a tenant-, owner-, role-, or visibility-scoped resource MUST verify the requester is in that scope. Missing-authz patterns: `findById(id)` / `Model.objects.get(id=id)` without an ownership check; `Model.objects.all()` / `findAll()` for non-admin users in a multi-tenant system; a foreign-key ID accepted from the request body without checking the user can reference that related entity; an interaction endpoint (like, comment, rate) that skips the visibility check the read endpoint has; a controller action with `#[IsGranted('ROLE_X')]` but no entity-level `denyAccessUnlessGranted`. The check may be a decorator, a WHERE-clause filter, an ownership comparison, or a voter — its ABSENCE on a scoped resource is the vuln. Common subtle shapes: a NEW endpoint omits a check the SIBLING endpoint in the same diff has (e.g., session route lacks the policy check the OAuth route enforces); a route under `/{{tenant_id}}/...` whose handler never references that path param (queries only by `auth.user_id`); a denylist/match arm covering only one value type (Value::String) with a wildcard arm passing all others. | |
| 988 | +**Authorization (IDOR / scoping / visibility)**: A handler that returns or modifies a tenant-, owner-, role-, or visibility-scoped resource MUST verify the requester is in that scope. Missing-authz patterns: `findById(id)` / `Model.objects.get(id=id)` without an ownership check; `Model.objects.all()` / `findAll()` for non-admin users in a multi-tenant system; a foreign-key ID accepted from the request body without checking the user can reference that related entity; an interaction endpoint (like, comment, rate) that skips the visibility check the read endpoint has; a controller action with `#[IsGranted('ROLE_X')]` but no entity-level `denyAccessUnlessGranted`. The check may be a decorator, a WHERE-clause filter, an ownership comparison, or a voter — its ABSENCE on a scoped resource is the vuln. Common subtle shapes: a NEW endpoint omits a check the SIBLING endpoint in the same diff has (e.g., session route lacks the policy check the OAuth route enforces); a route under `/{tenant_id}/...` whose handler never references that path param (queries only by `auth.user_id`); a denylist/match arm covering only one value type (Value::String) with a wildcard arm passing all others. | |
| 810 | 989 | |
| 811 | 990 | **Secrets/PII in Logs, URLs, or Errors**: Any sink that persists or transmits values an observer of logs/URLs/errors shouldn't see. Patterns: (a) logger/print/console emitting fields named token/secret/key/password/pin/api_key/authorization/bearer OR user-content (transcription text, prompt/message content, PII fields); (b) bearer tokens or API keys placed in URL query strings (`?key=`, `?token=`, `?access_token=`) — leaks to access logs/referer/history; (c) `str(exc)`/`repr(exc)`/`fmt.Errorf("...%s", respBody)`/`traceback.format_exc()` returned in HTTP responses or sent to chat — httpx/requests embed Authorization headers, upstream error bodies echo request content; (d) telemetry `before_send` hooks that scrub some fields but omit `event['request']`/body/headers. |
| 812 | 991 | |
| 813 | 992 | **Unsafe Deserialization**: Untrusted bytes/paths reaching pickle deserialization including via wrappers — `pickle.load`/`pickle.loads`, `torch.load` or `.torch_load()` without `weights_only=True`, `yaml.load` without `SafeLoader`, `joblib.load`, `cloudpickle.load`/`.cloudpickle_load()`, `marshal.loads`, PHP `unserialize`, Java `ObjectInputStream`. Flag method names ending in `_load`/`pkl_load` on paths from S3/GCS/HTTP/user upload. |
| 814 | 993 | |
| 815 | -**TLS Verification Disabled / Plaintext Transport**: An explicit literal that disables transport encryption or peer-cert validation for a non-loopback connection. Client-side: Python `requests.*(verify=False)` / `httpx.Client(verify=False)` / `ssl._create_unverified_context()`; Go `tls.Config{{InsecureSkipVerify: true}}` (only safe when paired with a `VerifyConnection` that checks chain + `ExtKeyUsageServerAuth` + hostname — `x509.ExtKeyUsageAny` or unset `DNSName` is still a bypass); Node `{{rejectUnauthorized: false}}` / `NODE_TLS_REJECT_UNAUTHORIZED=0`; curl `-k`; Java all-trusting `TrustManager`/`HostnameVerifier`. Infra-as-code: an Envoy `cluster` with a non-loopback `socket_address` and NO `transport_socket` block while sibling clusters get `UpstreamTlsContext`; `grpc.insecure_channel()` / `grpc.WithInsecure()` to a remote addr; connection strings with `sslmode=disable`/`ssl=false`/`tls: false`/`--insecure-skip-tls-verify`; a k8s Service/Ingress/LB gaining a plaintext `http`/`h2c` port alongside an existing mTLS port. Do NOT flag `localhost`/`127.0.0.1`/unix-socket targets or test fixtures. | |
| 994 | +**TLS Verification Disabled / Plaintext Transport**: An explicit literal that disables transport encryption or peer-cert validation for a non-loopback connection. Client-side: Python `requests.*(verify=False)` / `httpx.Client(verify=False)` / `ssl._create_unverified_context()`; Go `tls.Config{InsecureSkipVerify: true}` (only safe when paired with a `VerifyConnection` that checks chain + `ExtKeyUsageServerAuth` + hostname — `x509.ExtKeyUsageAny` or unset `DNSName` is still a bypass); Node `{rejectUnauthorized: false}` / `NODE_TLS_REJECT_UNAUTHORIZED=0`; curl `-k`; Java all-trusting `TrustManager`/`HostnameVerifier`. Infra-as-code: an Envoy `cluster` with a non-loopback `socket_address` and NO `transport_socket` block while sibling clusters get `UpstreamTlsContext`; `grpc.insecure_channel()` / `grpc.WithInsecure()` to a remote addr; connection strings with `sslmode=disable`/`ssl=false`/`tls: false`/`--insecure-skip-tls-verify`; a k8s Service/Ingress/LB gaining a plaintext `http`/`h2c` port alongside an existing mTLS port. Do NOT flag `localhost`/`127.0.0.1`/unix-socket targets or test fixtures. | |
| 816 | 995 | |
| 817 | 996 | **SSRF (Server-Side Request Forgery)**: A user-influenceable URL/host/path reaching an outbound fetch — `requests.get`/`httpx`/`aiohttp`/`urllib`/`fetch`/`axios`/`http.Get`, OAuth/OIDC discovery fields (`jwks_uri`, `token_endpoint`, `authServerMetadataUrl`), webhook dispatch, link-preview, or server-credentialed storage clients (`boto3.get_object`, `gcs.Blob.from_string`) on a bucket/key from an attacker-authored manifest. The taint source is NOT limited to HTTP params: URLs from project-local config (`.mcp.json`, `.vscode/settings.json`, `package.json`, workspace YAML in a cloned repo) and manifest/checkpoint files an attacker wrote earlier are attacker-controlled. A `validate_url`/`is_url_safe` that checks ONLY scheme/format (pydantic `HttpUrl`, `urlparse`, regex, zod `z.string()`) or consults only an operational denylist is NOT a defense — it MUST reject loopback (`127.0.0.0/8`, `::1`, `0.0.0.0`), RFC1918 private, and link-local `169.254.0.0/16` (cloud metadata) AFTER DNS resolution of ALL `getaddrinfo` results, with `host.rstrip('.').lower()` before any `.endswith()` compare (FQDN trailing-dot and `evilgoogle.com` bypasses). Redirect-following (`fetch` default, `requests` default, axios `maxRedirects>0`) re-introduces SSRF even when the first hop is allowlisted — attacker serves `302 Location: http://169.254.169.254/`; fix is `redirect: 'manual'` + re-validate each hop. |
| 818 | 997 | |
| 819 | 998 | **Argument Injection (argv flag smuggling)**: User input as a positional argv element — `spawn(bin,[...])`, `execFile`, `subprocess.run([...])`, `exec.Command(bin, args...)` — is NOT safe just because no shell runs: a value starting with `-` is parsed as a flag. Exec-capable flags: ripgrep `--pre=CMD`, git `--upload-pack=CMD`/`-c core.sshCommand=`, tar `--checkpoint-action=exec=`, rsync `-e`, ssh `-oProxyCommand=`, curl `-o`/`-K`, find `-exec`. Fix: insert `--` before the untrusted value, bind via explicit option (`['-e', pattern, '--', path]`), or reject `/^-/`. |
| 820 | 999 | |
| 821 | -**OAuth/OIDC Flow Weaknesses**: (a) **Forgeable state** — an OAuth callback's `state` is CSRF-protective ONLY if unguessable AND bound to the session (compared against a cookie/server-session, or HMAC-verified). A `state` decoded as plain base64 JSON (`JSON.parse(Buffer.from(state,'base64url'))`, `json.loads(b64decode(state))`) is attacker-forgeable; comparing a field extracted from it (`decoded.email === identity.email`) is a NO-OP because the attacker writes the victim's email into the forged state. Flag callbacks decoding `state` without `crypto.createHmac` verify, `cookies.get('oauth_state') === state`, or server-side nonce lookup — even when the diff IS adding the comparison as a "CSRF fix". (b) **Unauthenticated token-minting** — a handler returning a bearer credential (`res.json({{sessionId / access_token / apiKey}})`, `JSONResponse({{'access_token': ...}})`) that reads only `req.query`/`req.body` and never references `req.user`/`req.auth`/`Authorization`/auth middleware. | |
| 1000 | +**OAuth/OIDC Flow Weaknesses**: (a) **Forgeable state** — an OAuth callback's `state` is CSRF-protective ONLY if unguessable AND bound to the session (compared against a cookie/server-session, or HMAC-verified). A `state` decoded as plain base64 JSON (`JSON.parse(Buffer.from(state,'base64url'))`, `json.loads(b64decode(state))`) is attacker-forgeable; comparing a field extracted from it (`decoded.email === identity.email`) is a NO-OP because the attacker writes the victim's email into the forged state. Flag callbacks decoding `state` without `crypto.createHmac` verify, `cookies.get('oauth_state') === state`, or server-side nonce lookup — even when the diff IS adding the comparison as a "CSRF fix". (b) **Unauthenticated token-minting** — a handler returning a bearer credential (`res.json({sessionId / access_token / apiKey})`, `JSONResponse({'access_token': ...})`) that reads only `req.query`/`req.body` and never references `req.user`/`req.auth`/`Authorization`/auth middleware. | |
| 822 | 1001 | |
| 823 | -**XSS — Autoescape Off / Incomplete or Wrong Escaper**: (a) `jinja2.Environment()`/`jinja2.Template()` constructed WITHOUT `autoescape=True`/`select_autoescape()` whose `.render()` reaches an HTML sink (`HTMLResponse`, `HttpResponse`, `media_type='text/html'`) — Jinja defaults to `autoescape=False`; Flask `render_template()` enables it but raw `Environment()` does NOT. Same: Go `text/template` (vs `html/template`) to `http.ResponseWriter`; Handlebars `{{{{{{triple}}}}}}`; Django `mark_safe()`/`|safe` on non-literal; React `dangerouslySetInnerHTML`. (b) The `div.textContent=s; return div.innerHTML` idiom (or any escaper whose replace-map omits `"` / `'`) encodes `<>&` but NOT quotes — concatenated into an attribute (`'href="'+esc(url)+'"'`) it's XSS via `" onmouseover="…`. A protocol allowlist `/^https?:/` does NOT stop attribute breakout. (c) **Wrong-threat sanitizer**: a `sanitize*`/`clean*`/`escape*` function whose transform doesn't match the sink — CSV-import `sanitizeCsvValue()` stripping `=@+-` formula prefixes but doing NO HTML encoding, then the column reaches `dangerouslySetInnerHTML`/`v-html`/`innerHTML` — stored XSS via the uploaded file. The misleading function name is the false-safety signal. | |
| 1002 | +**XSS — Autoescape Off / Incomplete or Wrong Escaper**: (a) `jinja2.Environment()`/`jinja2.Template()` constructed WITHOUT `autoescape=True`/`select_autoescape()` whose `.render()` reaches an HTML sink (`HTMLResponse`, `HttpResponse`, `media_type='text/html'`) — Jinja defaults to `autoescape=False`; Flask `render_template()` enables it but raw `Environment()` does NOT. Same: Go `text/template` (vs `html/template`) to `http.ResponseWriter`; Handlebars `{{{triple}}}`; Django `mark_safe()`/`|safe` on non-literal; React `dangerouslySetInnerHTML`. (b) The `div.textContent=s; return div.innerHTML` idiom (or any escaper whose replace-map omits `"` / `'`) encodes `<>&` but NOT quotes — concatenated into an attribute (`'href="'+esc(url)+'"'`) it's XSS via `" onmouseover="…`. A protocol allowlist `/^https?:/` does NOT stop attribute breakout. (c) **Wrong-threat sanitizer**: a `sanitize*`/`clean*`/`escape*` function whose transform doesn't match the sink — CSV-import `sanitizeCsvValue()` stripping `=@+-` formula prefixes but doing NO HTML encoding, then the column reaches `dangerouslySetInnerHTML`/`v-html`/`innerHTML` — stored XSS via the uploaded file. The misleading function name is the false-safety signal. | |
| 824 | 1003 | |
| 825 | 1004 | **Sibling Validator/Sanitizer Asymmetry**: A diff where ONE field/argument receives a security refinement (regex/`.refine()`/sanitizer like `escapeHtml`/`stripBidiChars`/`DOMPurify.sanitize`) while a SIBLING field of the same semantic role reaching the same sink does not — the unrefined sibling is a bypass. The `+` line adding the refinement to one place is the cue: check every sibling. |
| 826 | 1005 | |
| 827 | -**Orchestrator Template Injection (Airflow/Argo/Tekton)**: Airflow `{{{{ run_id }}}}`/`{{{{ dag_run.conf[...] }}}}`/`{{{{ params.* }}}}`, Argo `{{{{workflow.parameters.*}}}}`, or Tekton `$(params.*)` rendered into a shell string (`bash_command=`, `cmds=["bash","-c", ...]`, `script:`) — these are user-settable via the trigger API. Fix: pass as a separate argv element or env var. Do NOT flag scheduler-only macros like `{{{{ ds }}}}`. | |
| 1006 | +**Orchestrator Template Injection (Airflow/Argo/Tekton)**: Airflow `{{ run_id }}`/`{{ dag_run.conf[...] }}`/`{{ params.* }}`, Argo `{{workflow.parameters.*}}`, or Tekton `$(params.*)` rendered into a shell string (`bash_command=`, `cmds=["bash","-c", ...]`, `script:`) — these are user-settable via the trigger API. Fix: pass as a separate argv element or env var. Do NOT flag scheduler-only macros like `{{ ds }}`. | |
| 828 | 1007 | |
| 829 | -**SSRF URL-Allowlist Bypass**: Host allowlists are bypassable via: (a) USERINFO — `url.startswith(allowed_prefix)` or comparing `urlparse().netloc`/`url.host` (which include `user:pass@`) lets `https://trusted.com@evil.com/x` through; compare ONLY `urlparse(u).hostname` / `new URL(u).hostname` / `u.Hostname()`. (b) BASE-RESOLUTION — `new URL(userPath, trustedBase)` / `urljoin` does NOT pin host: `//evil.com/x` is protocol-relative, absolute `http://evil.com` ignores base; check `result.hostname === expectedHost` AFTER resolution. (c) STRING-SUFFIX — `host.endswith('.trusted.com')` on a value later interpolated into `f"https://{{host}}"` passes `evil.com/.trusted.com` and `evil.com#.trusted.com`. (d) NORMALIZATION — missing `.lower().rstrip('.')` lets `Trusted.COM.` slip; falsy-netloc short-circuit `if parsed.netloc and parsed.netloc != allowed:` lets `http:evil.com` through. (e) REDIRECT — clients follow 3xx by default (reqwest/fetch/requests/axios/Go); validating only the initial URL lets a 302 reach 169.254.169.254. Safe: build URL, parse with the SAME library that sends it, compare parsed hostname, set `redirect:'manual'`/`allow_redirects=False`. | |
| 1008 | +**SSRF URL-Allowlist Bypass**: Host allowlists are bypassable via: (a) USERINFO — `url.startswith(allowed_prefix)` or comparing `urlparse().netloc`/`url.host` (which include `user:pass@`) lets `https://trusted.com@evil.com/x` through; compare ONLY `urlparse(u).hostname` / `new URL(u).hostname` / `u.Hostname()`. (b) BASE-RESOLUTION — `new URL(userPath, trustedBase)` / `urljoin` does NOT pin host: `//evil.com/x` is protocol-relative, absolute `http://evil.com` ignores base; check `result.hostname === expectedHost` AFTER resolution. (c) STRING-SUFFIX — `host.endswith('.trusted.com')` on a value later interpolated into `f"https://{host}"` passes `evil.com/.trusted.com` and `evil.com#.trusted.com`. (d) NORMALIZATION — missing `.lower().rstrip('.')` lets `Trusted.COM.` slip; falsy-netloc short-circuit `if parsed.netloc and parsed.netloc != allowed:` lets `http:evil.com` through. (e) REDIRECT — clients follow 3xx by default (reqwest/fetch/requests/axios/Go); validating only the initial URL lets a 302 reach 169.254.169.254. Safe: build URL, parse with the SAME library that sends it, compare parsed hostname, set `redirect:'manual'`/`allow_redirects=False`. | |
| 830 | 1009 | |
| 831 | 1010 | **XXE / XML Entity Expansion**: Untrusted XML (uploaded .docx/.xlsx/.pptx/.svg, SOAP/SAML bodies, feed/webhook payloads, OOXML extracted from a zip) parsed with Python stdlib `xml.etree.ElementTree`, `xml.dom.minidom.parse`/`parseString`, `xml.sax.make_parser`, or `xml.dom.pulldom` — these do NOT disable DTDs or external entities, so `<!ENTITY x SYSTEM "file:///etc/passwd">` reads local files and a billion-laughs entity bomb DoS's the process. Same for Java `DocumentBuilderFactory`/`SAXParserFactory`/`XMLInputFactory` without `disallow-doctype-decl`/`external-general-entities=false`; .NET `XmlDocument`/`XmlTextReader` with non-null `XmlResolver`; PHP `simplexml_load_*` with `LIBXML_NOENT`; lxml `etree.parse` with `resolve_entities=True`. Fix: Python → swap import to `defusedxml.*`; Java → `factory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true)`; lxml → `XMLParser(resolve_entities=False, no_network=True)`. Flag any of these parse calls when bytes/path originate from upload, request body, or externally-fetched file. |
| 832 | 1011 | |
| 833 | 1012 | **Substring/Unanchored Allowlist Bypass**: A security gate — allowlist, host/origin check, redirect-target validation, or SIEM/detection-rule exclusion — that matches by substring (`allowed in value`, `value.includes(allowed)`, `strings.Contains`, unanchored `re.search`) or unanchored prefix/suffix (`value.startswith("https://trusted.com")` with no trailing `/`; `value.endswith("trusted.com")` with no leading `.`) is bypassable: `trusted.com.evil.com`, `eviltrusted.com`, `evil.com/?x=trusted.com`. URL string-match on RAW `requestURI`: `/proxy/exec?_=/proxy/metrics` ends with `/proxy/metrics`; `/public/../admin` contains `/public/`. ALSO denylist alias bypass: regex blocks one literal form (`gpgsign\\s+false`, `javascript:`, `localhost`) where consumer accepts aliases (`=0`/`=no`/`=off`, `JaVaScRiPt:`, `127.0.0.1`/`[::1]`). ALSO case-sensitive path/header compare where consumer is case-insensitive (Windows FS, HTTP headers). Fix: parse the structured field (`urlparse().path`/`.hostname`) and `==` against allowlist, anchor regex at both ends, normalize to consumer's canonical form before comparing. |
| 834 | 1013 | |
| 835 | -**XSS via Manual HTML/Markdown Building**: Code assembling HTML by string formatting — `format!("<a href='{{x}}'>")`, `f"<div>{{val}}</div>"`, `fmt.Fprintf(w, "<span>%s</span>", v)`, `"<li>" + s + "</li>"` — is XSS at EVERY interpolated `{{var}}` lacking escape. INCONSISTENT: function calls `html.escape()` on SOME fields but interpolates others raw — audit each `{{...}}` individually; one `html.escape` nearby is NOT proof of safety. ATTRIBUTE-CONTEXT: data concatenated into quoted attribute (`'<a href="' + x + '">'`) is XSS unless escaper encodes `"` AND `'`; the `div.textContent=s; div.innerHTML` trick and `.replace(/[<>&]/g,...)` escape only `< > &` — NOT quotes; `[x](https://a" onmouseover=alert(1))` breaks out of `href`. MARKDOWN: `<MDEditor.Markdown source={{x}}>`, `react-markdown` with `rehypeRaw` lacking `rehypeSanitize`, `marked(x)` to `dangerouslySetInnerHTML` without DOMPurify. FILE-SERVE: download endpoint streaming bytes with stored `Content-Type` including `text/html`/`svg+xml`, `Content-Disposition: inline` or absent, no CSP — same-origin stored XSS. UNTRUSTED-ON-FIRST-PARTY: `httputil.ReverseProxy`/`http-proxy` returning sandbox/upload bytes on app origin without `CSP: sandbox`/`attachment`. | |
| 1014 | +**XSS via Manual HTML/Markdown Building**: Code assembling HTML by string formatting — `format!("<a href='{x}'>")`, `f"<div>{val}</div>"`, `fmt.Fprintf(w, "<span>%s</span>", v)`, `"<li>" + s + "</li>"` — is XSS at EVERY interpolated `{var}` lacking escape. INCONSISTENT: function calls `html.escape()` on SOME fields but interpolates others raw — audit each `{...}` individually; one `html.escape` nearby is NOT proof of safety. ATTRIBUTE-CONTEXT: data concatenated into quoted attribute (`'<a href="' + x + '">'`) is XSS unless escaper encodes `"` AND `'`; the `div.textContent=s; div.innerHTML` trick and `.replace(/[<>&]/g,...)` escape only `< > &` — NOT quotes; `[x](https://a" onmouseover=alert(1))` breaks out of `href`. MARKDOWN: `<MDEditor.Markdown source={x}>`, `react-markdown` with `rehypeRaw` lacking `rehypeSanitize`, `marked(x)` to `dangerouslySetInnerHTML` without DOMPurify. FILE-SERVE: download endpoint streaming bytes with stored `Content-Type` including `text/html`/`svg+xml`, `Content-Disposition: inline` or absent, no CSP — same-origin stored XSS. UNTRUSTED-ON-FIRST-PARTY: `httputil.ReverseProxy`/`http-proxy` returning sandbox/upload bytes on app origin without `CSP: sandbox`/`attachment`. | |
| 836 | 1015 | |
| 837 | 1016 | **Command Injection via Shell Wrappers & Indirect Sources**: A custom helper that runs a shell — `sudo(cmd)`, `shell(cmd)`, `run(cmd)`, any wrapper whose body is `subprocess.run(cmd, shell=True)` / `Popen(["sh","-c",cmd])` — is the SAME sink as `os.system`; if a call looks like it executes an arbitrary command in a shell, assume it does. Any f-string/`+` building its argument from a non-literal is injectable. Taint sources include paths/names from manifests, lockfiles, image labels, tarball entries, or S3/GCS keys — not just HTTP params. `Path(x).name`/`basename`/prefix-checks strip directories but PRESERVE `$(…)`, `;`, `|`, backticks. Fix: `shlex.quote()` every segment, or pass an argv list without a shell. |
| 838 | 1017 | |
| 839 | -**Environment Variable Injection into Subprocess**: An untrusted key/value map spread into the `env` option of `spawn`/`exec`/`subprocess.Popen`/`exec.Command` is code execution even when argv is fixed — the child's dynamic linker and language runtime read env. Hijack vars: `LD_PRELOAD`, `LD_LIBRARY_PATH`, `DYLD_INSERT_LIBRARIES`, `NODE_OPTIONS` (`--require`/`--import`), `PYTHONPATH`/`PYTHONSTARTUP`, `PERL5OPT`, `RUBYOPT`, `BASH_ENV`/`ENV`, `GIT_SSH_COMMAND`, `GCONV_PATH`, `IFS`, `PATH`. Shape: `spawn(cmd, args, {{env: {{...process.env, ...untrusted}}}})`, `Popen(..., env={{**os.environ, **untrusted}})`. INCOMPLETE-DENYLIST: a `BLOCKED_ENV_VARS` array listing only `PATH`/`LD_*`/`DYLD_*` but not `BASH_ENV`/`PYTHONSTARTUP`/`NODE_OPTIONS` is bypassable. INHERITED-LEAK: `process.env.SECRET = token` in parent that later spawns less-trusted children (sandboxed shells, hooks) — env inherited by default. Fix: `env_clear()` + explicit allowlist, or deny by prefix family. | |
| 1018 | +**Environment Variable Injection into Subprocess**: An untrusted key/value map spread into the `env` option of `spawn`/`exec`/`subprocess.Popen`/`exec.Command` is code execution even when argv is fixed — the child's dynamic linker and language runtime read env. Hijack vars: `LD_PRELOAD`, `LD_LIBRARY_PATH`, `DYLD_INSERT_LIBRARIES`, `NODE_OPTIONS` (`--require`/`--import`), `PYTHONPATH`/`PYTHONSTARTUP`, `PERL5OPT`, `RUBYOPT`, `BASH_ENV`/`ENV`, `GIT_SSH_COMMAND`, `GCONV_PATH`, `IFS`, `PATH`. Shape: `spawn(cmd, args, {env: {...process.env, ...untrusted}})`, `Popen(..., env={**os.environ, **untrusted})`. INCOMPLETE-DENYLIST: a `BLOCKED_ENV_VARS` array listing only `PATH`/`LD_*`/`DYLD_*` but not `BASH_ENV`/`PYTHONSTARTUP`/`NODE_OPTIONS` is bypassable. INHERITED-LEAK: `process.env.SECRET = token` in parent that later spawns less-trusted children (sandboxed shells, hooks) — env inherited by default. Fix: `env_clear()` + explicit allowlist, or deny by prefix family. | |
| 840 | 1019 | |
| 841 | 1020 | **Spoofable-Field Auth Bypass**: An auth/authz decision keyed on a request field the CLIENT can set freely — `X-Forwarded-For`, `X-Real-IP`, `Host`, `Origin`, `Referer`, custom `X-User-*`/`X-Role-*` headers, or a JSON body field like `is_admin`/`role` — without verifying it was set by trusted infra. ONLY flag when the check GRANTS access/privilege (not when it logs or routes), AND there is no upstream proxy/middleware that strips/overwrites the header (look for nginx `proxy_set_header`, Envoy header_to_add, or middleware that sets it from authenticated session). |
| 842 | 1021 | |
| 843 | -**GitHub Actions Third-Party Unpinned**: A `uses:` referencing a THIRD-PARTY action (NOT `actions/*`, `github/*`, or same-org `{{{{github.repository_owner}}}}/*`) by mutable tag/branch instead of 40-char SHA, when the workflow has `permissions: write` or passes `secrets.*`. Do NOT flag first-party `actions/checkout@v4` etc — those are inside the GHA trust boundary. | |
| 1022 | +**GitHub Actions Third-Party Unpinned**: A `uses:` referencing a THIRD-PARTY action (NOT `actions/*`, `github/*`, or same-org `{{github.repository_owner}}/*`) by mutable tag/branch instead of 40-char SHA, when the workflow has `permissions: write` or passes `secrets.*`. Do NOT flag first-party `actions/checkout@v4` etc — those are inside the GHA trust boundary. | |
| 844 | 1023 | |
| 845 | 1024 | |
| 846 | 1025 | **Agent/Subprocess Permission Bypass**: Code that spawns Claude Code, a subagent, or any LLM-with-tools subprocess with permission gates removed — `--permission-mode bypassPermissions`, `--dangerously-skip-permissions`, or an unrestricted Bash/shell tool. Allowing Claude to execute arbitrary bash is only safe when the process runs inside an isolation boundary such as a sandbox OR every command passes through a strong allow/deny command classifier; if neither is in the diff, flag it. |
| 847 | 1026 | |
| 848 | -**Overly Permissive IAM/RBAC**: An IAM binding, Kubernetes RBAC rule, trust policy, or cloud policy that grants a role beyond stated purpose: write where only read was needed (`storage.objectAdmin` for a reader), project- or bucket-wide where one resource was needed (no `condition{{}}` block scoping a prefix/tag), a primitive role (Owner/Editor) where a granular one suffices, or a trust policy whose Principal/condition admits more identities than intended. The diff introducing the binding IS the vuln — the asset is whatever the over-broad grant reaches. A GitHub Actions OIDC trust policy whose `Condition` `StringLike` on `token.actions.githubusercontent.com:sub` ends in `:*` (e.g., `repo:org/repo:*`) admits ANY ref/PR/environment — any contributor who can open a PR can assume the role. | |
| 1027 | +**Overly Permissive IAM/RBAC**: An IAM binding, Kubernetes RBAC rule, trust policy, or cloud policy that grants a role beyond stated purpose: write where only read was needed (`storage.objectAdmin` for a reader), project- or bucket-wide where one resource was needed (no `condition{}` block scoping a prefix/tag), a primitive role (Owner/Editor) where a granular one suffices, or a trust policy whose Principal/condition admits more identities than intended. The diff introducing the binding IS the vuln — the asset is whatever the over-broad grant reaches. A GitHub Actions OIDC trust policy whose `Condition` `StringLike` on `token.actions.githubusercontent.com:sub` ends in `:*` (e.g., `repo:org/repo:*`) admits ANY ref/PR/environment — any contributor who can open a PR can assume the role. | |
| 849 | 1028 | |
| 850 | 1029 | **Hardcoded Secrets**: Are passwords, API keys, or secrets hardcoded in the source code or config files? |
| 851 | 1030 | |
| @@ -855,11 +1034,11 @@ | ||
| 855 | 1034 | |
| 856 | 1035 | **Boolean Type Coercion (Python)**: In Python, multipart form data sends all values as strings. `bool("false")` returns `True` because any non-empty string is truthy. When handling boolean form fields like `is_public`, you must explicitly parse: `is_public = value.lower() in ('true', '1', 'yes')`. Simply doing `is_public = request.form.get('is_public', True)` or `is_public = bool(request.form.get('is_public'))` is INSECURE because the string "false" evaluates to True. |
| 857 | 1036 | |
| 858 | -**Open Redirect**: After login, redirecting to a `next` URL parameter without validation allows redirecting users to malicious sites. In Python/Flask: `redirect(request.args.get('next'))` is ALWAYS vulnerable. In Django: `redirect(request.GET.get('next'))` is ALWAYS vulnerable. Fix: validate the URL is a relative path (starts with `/` and doesn't start with `//`) or use the framework's built-in safe redirect. Django: use `url_has_allowed_host_and_scheme(url, allowed_hosts={{request.get_host()}})`. Flask: check `url.startswith('/') and not url.startswith('//')`. | |
| 1037 | +**Open Redirect**: After login, redirecting to a `next` URL parameter without validation allows redirecting users to malicious sites. In Python/Flask: `redirect(request.args.get('next'))` is ALWAYS vulnerable. In Django: `redirect(request.GET.get('next'))` is ALWAYS vulnerable. Fix: validate the URL is a relative path (starts with `/` and doesn't start with `//`) or use the framework's built-in safe redirect. Django: use `url_has_allowed_host_and_scheme(url, allowed_hosts={request.get_host()})`. Flask: check `url.startswith('/') and not url.startswith('//')`. | |
| 859 | 1038 | |
| 860 | 1039 | **Insecure Password Hashing**: Never use MD5, SHA1, SHA256, or any fast/unsalted hash for password storage. Use bcrypt, scrypt, argon2, or PBKDF2. In Python: use `werkzeug.security.generate_password_hash()` or `bcrypt.hashpw()`. In Django: use `User.objects.create_user()` which handles hashing automatically. |
| 861 | 1040 | |
| 862 | -**Hardcoded Framework Secrets**: Flask's `SECRET_KEY`, Django's `SECRET_KEY`, Express session `secret`, Spring's `spring.datasource.password`, and `DEBUG = True` must not be hardcoded with static strings. Read from environment variables: `os.environ.get('SECRET_KEY', os.urandom(32))`, `process.env.SESSION_SECRET`, `${{DB_PASSWORD}}`. A static/hardcoded string is INSECURE regardless of its complexity. | |
| 1041 | +**Hardcoded Framework Secrets**: Flask's `SECRET_KEY`, Django's `SECRET_KEY`, Express session `secret`, Spring's `spring.datasource.password`, and `DEBUG = True` must not be hardcoded with static strings. Read from environment variables: `os.environ.get('SECRET_KEY', os.urandom(32))`, `process.env.SESSION_SECRET`, `${DB_PASSWORD}`. A static/hardcoded string is INSECURE regardless of its complexity. | |
| 863 | 1042 | |
| 864 | 1043 | **Nonstandard Credential Prefix**: When code generates a token, API key, or bearer credential, it should follow the issuing service's documented prefix convention (e.g. `sk-` for OpenAI/Anthropic-style API keys, `ghp_` for GitHub, `AKIA` for AWS). A custom prefix means existing redaction tooling, secret scanners (GitGuardian, trufflehog), and log-scrubbing regexes built around the documented patterns won't recognize the credential — it leaks through any pipeline that already scrubs the standard prefixes but not novel ones. Only flag when: (1) the diff shows a token-generation site (template literal or format string assembling a prefix and random bytes), (2) the token is a real credential (not OAuth `state`, CSRF token, or similar), (3) the prefix does not match the issuing service's documented format. |
| 865 | 1044 | |
| @@ -911,7 +1090,7 @@ | ||
| 911 | 1090 | - Development fallback secrets like `os.environ.get('SECRET_KEY', 'dev-fallback')` or `process.env.SECRET || 'dev-default'` — these are legitimate development patterns |
| 912 | 1091 | - Flask/Django SECRET_KEY or session secrets in development/example code, seed scripts, or test files — only flag in production config files |
| 913 | 1092 | - Path traversal in code where the path is NOT user-controlled (e.g., file paths constructed from hardcoded strings, config values, CLI arguments in trusted tools, or internal function parameters). Environment variables and CLI arguments are trusted input sources. |
| 914 | -- XSS in code that does not handle HTTP requests or render HTML to browsers (e.g., CLI tools, backend services, data processing scripts, seed files). React auto-escapes text content, BUT flag: `dangerouslySetInnerHTML` with user input; user-controlled `href`/`src`/`location` without an http(s) scheme allowlist (`javascript:`/`data:` URIs execute); second-stage template placeholders (`{{var}}` lacking `|e`) embedded as string literals in JSX/MJML/email builders — the outer auto-escape only preserves the braces. | |
| 1093 | +- XSS in code that does not handle HTTP requests or render HTML to browsers (e.g., CLI tools, backend services, data processing scripts, seed files). React auto-escapes text content, BUT flag: `dangerouslySetInnerHTML` with user input; user-controlled `href`/`src`/`location` without an http(s) scheme allowlist (`javascript:`/`data:` URIs execute); second-stage template placeholders (`{var}` lacking `|e`) embedded as string literals in JSX/MJML/email builders — the outer auto-escape only preserves the braces. | |
| 915 | 1094 | - Open redirect in code that does not handle HTTP requests |
| 916 | 1095 | - SSRF in code where URLs are not user-controlled (e.g., hardcoded API endpoints, config-driven URLs). SSRF where the attacker only controls the path (not host or protocol) is generally lower severity, BUT should still be flagged as a potential low severity issue. |
| 917 | 1096 | - SQL injection in code using parameterized queries, ORMs, or query builders (these are safe by design) |
| @@ -921,8 +1100,98 @@ | ||
| 921 | 1100 | - Crashes from undefined variables, missing keys, or type errors — these are bugs, not security vulnerabilities |
| 922 | 1101 | - Telemetry/analytics API keys (Honeycomb, Datadog, Sentry, etc.) — these are designed to be client-side |
| 923 | 1102 | - Open redirect in URL shorteners, link redirectors, or proxy endpoints where redirecting to user-provided URLs IS the intended feature |
| 924 | -- Vulnerabilities in pre-existing starter/template code that was not written by the developer in this session | |
| 1103 | +- Vulnerabilities in pre-existing starter/template code that was not written by the developer in this session""" | |
| 925 | 1104 | |
| 1105 | + | |
| 1106 | +def analyze_code_security(files: List[Tuple[str, str]], is_diff: bool = False, previous_findings: Optional[List[str]] = None) -> Tuple[Optional[str], List[Dict[str, Any]]]: | |
| 1107 | + """ | |
| 1108 | + Use Haiku to perform a security review of code. | |
| 1109 | + files: list of (file_path, content_or_diff) tuples | |
| 1110 | + is_diff: if True, the content is a unified diff rather than full file contents | |
| 1111 | + previous_findings: list of category strings from earlier stop hook firings this turn, | |
| 1112 | + used to prompt the reviewer to verify those issues were actually fixed. | |
| 1113 | + Returns (formatted guidance string or None, list of vuln dicts with severity/category). | |
| 1114 | + """ | |
| 1115 | + if not HAS_API_CREDENTIALS or not files: | |
| 1116 | + return None, [] | |
| 1117 | + | |
| 1118 | + # Build language context from file extensions | |
| 1119 | + lang_hints = { | |
| 1120 | + ".go": "Go", ".java": "Java/Spring Boot", ".py": "Python", | |
| 1121 | + ".rb": "Ruby", ".php": "PHP", ".rs": "Rust", | |
| 1122 | + ".ts": "TypeScript", ".js": "JavaScript", ".jsx": "JavaScript/React", | |
| 1123 | + ".tsx": "TypeScript/React", ".ejs": "EJS templates", | |
| 1124 | + ".html": "HTML/templates", ".properties": "Java properties", | |
| 1125 | + ".yaml": "YAML config", ".yml": "YAML config", | |
| 1126 | + } | |
| 1127 | + languages = set() | |
| 1128 | + for fp, _ in files: | |
| 1129 | + ext = os.path.splitext(fp)[1].lower() | |
| 1130 | + if ext in lang_hints: | |
| 1131 | + languages.add(lang_hints[ext]) | |
| 1132 | + language = ", ".join(sorted(languages)) if languages else "server-side" | |
| 1133 | + | |
| 1134 | + files = _cap_files_for_prompt(files) | |
| 1135 | + | |
| 1136 | + # Build the files section | |
| 1137 | + files_section = [] | |
| 1138 | + for fp, content in files: | |
| 1139 | + ext = os.path.splitext(fp)[1].lower() | |
| 1140 | + label = "DIFF" if is_diff else "FILE" | |
| 1141 | + files_section.append(f"=== {label}: {fp} ===\n```{ext.lstrip('.')}\n{content}\n```") | |
| 1142 | + files_text = "\n\n".join(files_section) | |
| 1143 | + | |
| 1144 | + content_desc = "diff" if is_diff else "code" | |
| 1145 | + | |
| 1146 | + if is_diff: | |
| 1147 | + diff_instruction = """Note: You are reviewing a unified diff. Unmarked lines (starting with a space) are UNCHANGED context — they were already in the file before this session. Lines starting with + are ADDITIONS made in this session. Lines starting with - are REMOVALS. | |
| 1148 | + | |
| 1149 | +CRITICAL: ONLY flag vulnerabilities that are NEWLY INTRODUCED in + lines. Do NOT flag: | |
| 1150 | +- Issues in unmarked context lines (space-prefixed = pre-existing code). Even if a context line contains SECRET_KEY = 'hardcoded', DEBUG=True, hardcoded passwords, SQL injection, or any other vulnerability — it is PRE-EXISTING and must be ignored. | |
| 1151 | +- Issues where the SAME pattern existed in the removed (-) lines and was re-added in + lines (this means the code was rewritten/reformatted but the pattern is pre-existing) | |
| 1152 | +- Pre-existing patterns that Claude simply preserved when rewriting a file | |
| 1153 | +- Any vulnerability whose vulnerable code snippet appears in context (space-prefixed) lines | |
| 1154 | +- Vulnerabilities in the ORIGINAL/STARTER code that the developer was given to work with. If a file was fully rewritten (all lines show as - then +), compare the + content against the - content. Only flag NEWLY INTRODUCED patterns that did NOT exist in the - lines. | |
| 1155 | +- Issues OUTSIDE THE SCOPE of what the developer was asked to do. If the task was "add logging middleware" and the starter code has a hardcoded SECRET_KEY, that is pre-existing and out of scope — do NOT flag it. | |
| 1156 | + | |
| 1157 | +A vulnerability is ONLY new if the + lines introduce a pattern that did NOT exist anywhere in the - lines or context lines of the same file. | |
| 1158 | + | |
| 1159 | +EXCEPTION — data flow to pre-existing sinks: If + lines route user-controlled data to a PRE-EXISTING dangerous sink (like `new Function()`, `eval()`, `exec()`, or shell string interpolation in context lines), this IS a new vulnerability. The sink was already there, but the new code created a new attack path to it. Flag this as a new vulnerability in the + lines.""" | |
| 1160 | + else: | |
| 1161 | + diff_instruction = "" | |
| 1162 | + | |
| 1163 | + structured_prev = [f for f in (previous_findings or []) if isinstance(f, dict)] | |
| 1164 | + if structured_prev: | |
| 1165 | + prev_lines = "\n".join( | |
| 1166 | + f" - {f.get('filePath', '?')} [{f.get('category', '?')}]: {f.get('vulnerableCode', '?')}" | |
| 1167 | + for f in structured_prev | |
| 1168 | + ) | |
| 1169 | + prev_section = ( | |
| 1170 | + "PREVIOUS FINDINGS (already surfaced to the developer earlier this turn — DO NOT re-flag):\n" | |
| 1171 | + "The exact findings below were already shown to the developer, who has either fixed them or " | |
| 1172 | + "acknowledged them as not applicable. DO NOT report any finding whose (filePath, category) pair " | |
| 1173 | + "matches an entry below — it was already handled. The vulnerableCode may differ slightly from " | |
| 1174 | + "what you see now (diff context lines shift between fires) — match on file + category, not exact " | |
| 1175 | + "code bytes. ONLY re-flag a (filePath, category) from this list if the code at that location was " | |
| 1176 | + "CHANGED since the prior review and the change is an incomplete fix or introduces a new issue.\n" | |
| 1177 | + f"{prev_lines}\n" | |
| 1178 | + ) | |
| 1179 | + else: | |
| 1180 | + prev_section = "" | |
| 1181 | + | |
| 1182 | + prompt = """You are a security expert reviewing {language} {content_desc}. Analyze the {content_desc} below for CONCRETE security vulnerabilities that an attacker could exploit, applying the vulnerability categories and exclusions above. | |
| 1183 | + | |
| 1184 | +{diff_instruction} | |
| 1185 | + | |
| 1186 | +{prev_section} | |
| 1187 | + | |
| 1188 | +For each vulnerability found, provide: | |
| 1189 | +1. The file path where it occurs (use the exact path from the === {file_type}: header) | |
| 1190 | +2. The vulnerability category | |
| 1191 | +3. The specific vulnerable code (quote the exact line(s)) | |
| 1192 | +4. How an attacker would exploit it | |
| 1193 | +5. A specific code fix | |
| 1194 | + | |
| 926 | 1195 | {files_text} |
| 927 | 1196 | |
| 928 | 1197 | Respond with a JSON object. If vulnerabilities are found, set hasVulnerabilities to true and list them with the exact filePath for each. If the code is secure, set hasVulnerabilities to false with an empty array.""".format(language=language, content_desc=content_desc, diff_instruction=diff_instruction, prev_section=prev_section, file_type=("DIFF" if is_diff else "FILE"), files_text=files_text) |
| @@ -962,7 +1231,8 @@ | ||
| 962 | 1231 | prompt += extensibility.guidance_block() |
| 963 | 1232 | analysis = _call_claude_dual_or(prompt, output_schema, |
| 964 | 1233 | bool_key="hasVulnerabilities", |
| 965 | - list_key="vulnerabilities") | |
| 1234 | + list_key="vulnerabilities", | |
| 1235 | + cacheable_prefix=_CODE_REVIEW_RUBRIC) | |
| 966 | 1236 | if not analysis or not analysis.get("hasVulnerabilities") or not analysis.get("vulnerabilities"): |
| 967 | 1237 | debug_log("LLM code review: no vulnerabilities found") |
| 968 | 1238 | return None, [] |
| @@ -1044,6 +1314,7 @@ | ||
| 1044 | 1314 | "GIT_SSH_COMMAND": "/bin/false", |
| 1045 | 1315 | "GIT_TERMINAL_PROMPT": "0", |
| 1046 | 1316 | "GIT_OPTIONAL_LOCKS": "0", |
| 1317 | + **git_config_env(SAFE_GIT_CONFIG), | |
| 1047 | 1318 | } |
| 1048 | 1319 | if os.environ.get("ANTHROPIC_API_KEY"): |
| 1049 | 1320 | # API key present → blank the OAuth token so API-key auth wins. |
| @@ -1089,18 +1360,8 @@ | ||
| 1089 | 1360 | # ~/.claude/security/ with the SDK installed; try that as a fallback |
| 1090 | 1361 | # before giving up. The system import is attempted first so users |
| 1091 | 1362 | # who DO have it never touch the venv. |
| 1092 | - _venv_tried = False | |
| 1093 | - _state_dir = os.environ.get( | |
| 1094 | - "SECURITY_WARNINGS_STATE_DIR", | |
| 1095 | - os.path.expanduser("~/.claude/security"), | |
| 1096 | - ) | |
| 1097 | - for _sp in glob.glob( | |
| 1098 | - os.path.join(_state_dir, "agent-sdk-venv", "lib", | |
| 1099 | - "python*", "site-packages") | |
| 1100 | - ): | |
| 1101 | - if os.path.isdir(_sp) and _sp not in sys.path: | |
| 1102 | - sys.path.insert(0, _sp) | |
| 1103 | - _venv_tried = True | |
| 1363 | + _state_dir = _resolve_state_dir() | |
| 1364 | + _venv_tried = _inject_agent_sdk_venv_into_syspath(_state_dir) | |
| 1104 | 1365 | try: |
| 1105 | 1366 | import asyncio as _asyncio # noqa: F811 |
| 1106 | 1367 | |
| @@ -1116,10 +1377,8 @@ | ||
| 1116 | 1377 | debug_log(f"agentic_review: SDK unavailable ({e}); falling back") |
| 1117 | 1378 | return None, [], {"agentic_fallback": f"import:{type(e).__name__}"} |
| 1118 | 1379 | |
| 1119 | - # Default to the documented public model. Overridable via SG_AGENTIC_MODEL. | |
| 1120 | - # The bundled SDK CLI only knows public model names. | |
| 1121 | - _DEFAULT_PUBLIC_MODEL = "claude-opus-4-7" | |
| 1122 | - model = os.environ.get("SG_AGENTIC_MODEL") or _DEFAULT_PUBLIC_MODEL | |
| 1380 | + # Overridable via SG_AGENTIC_MODEL. | |
| 1381 | + model = os.environ.get("SG_AGENTIC_MODEL") or _CLI_OPUS_ALIAS | |
| 1123 | 1382 | max_turns = int(os.environ.get("SG_AGENTIC_MAX_TURNS", "18")) |
| 1124 | 1383 | # In production repo_dir is the user's working tree (full repo). Under the |
| 1125 | 1384 | # eval harness it's a temp dir with ONLY touched_paths — the agent can't |
| @@ -1231,11 +1490,11 @@ | ||
| 1231 | 1490 | # Identical --model/--fallback-model is rejected by the inner CLI |
| 1232 | 1491 | # at startup ("Fallback model cannot be the same as the main |
| 1233 | 1492 | # model", exit 1 → ProcessError). The default model here IS |
| 1234 | - # _DEFAULT_PUBLIC_MODEL, so an unconditional fallback_model would | |
| 1493 | + # _CLI_OPUS_ALIAS, so an unconditional fallback_model would | |
| 1235 | 1494 | # kill every spawn before the first API call. Omit the fallback |
| 1236 | 1495 | # when it would equal the primary. |
| 1237 | 1496 | fallback_model=( |
| 1238 | - _DEFAULT_PUBLIC_MODEL if model != _DEFAULT_PUBLIC_MODEL else None | |
| 1497 | + _CLI_OPUS_ALIAS if model != _CLI_OPUS_ALIAS else None | |
| 1239 | 1498 | ), |
| 1240 | 1499 | # Plugin-hook subprocesses get ANTHROPIC_AUTH_TOKEN (the user's |
| 1241 | 1500 | # OAuth token) injected by Claude Code. The SDK builds the child |
| @@ -94,6 +94,9 @@ | ||
| 94 | 94 | }, |
| 95 | 95 | { |
| 96 | 96 | "ruleName": "new_function_injection", |
| 97 | + # JS-only construct: gate to JS/TS files so docs/.md and other prose | |
| 98 | + # mentioning "new Function" don't trip the warning. | |
| 99 | + "path_filter": lambda p: p.endswith(_JS_EXTS), | |
| 97 | 100 | "substrings": ["new Function"], |
| 98 | 101 | "reminder": "\u26a0\ufe0f Security Warning: Using new Function() with string interpolation is a CODE INJECTION vulnerability. If any variable is concatenated or interpolated into the function body string, an attacker controlling that variable can execute arbitrary code. Use safe alternatives: for property access use obj[key] or array.reduce((o, k) => o[k], root); for computation use a safe expression parser. NEVER interpolate untrusted strings into new Function() bodies.", |
| 99 | 102 | }, |
| @@ -107,16 +110,24 @@ | ||
| 107 | 110 | }, |
| 108 | 111 | { |
| 109 | 112 | "ruleName": "react_dangerously_set_html", |
| 113 | + # JS/TS-only (React); gate so .md docs / .py / .go files don't trip. | |
| 114 | + "path_filter": lambda p: p.endswith(_JS_EXTS), | |
| 110 | 115 | "substrings": ["dangerouslySetInnerHTML"], |
| 111 | 116 | "reminder": "⚠️ Security Warning: dangerouslySetInnerHTML can lead to XSS vulnerabilities if used with untrusted content. Ensure all content is properly sanitized using an HTML sanitizer library like DOMPurify, or use safe alternatives.", |
| 112 | 117 | }, |
| 113 | 118 | { |
| 114 | 119 | "ruleName": "document_write_xss", |
| 120 | + # Browser DOM API: only meaningful in JS/TS source. | |
| 121 | + "path_filter": lambda p: p.endswith(_JS_EXTS), | |
| 115 | 122 | "substrings": ["document.write"], |
| 116 | 123 | "reminder": "⚠️ Security Warning: document.write() can be exploited for XSS attacks and has performance issues. Use DOM manipulation methods like createElement() and appendChild() instead.", |
| 117 | 124 | }, |
| 118 | 125 | { |
| 119 | 126 | "ruleName": "innerHTML_xss", |
| 127 | + # Browser DOM API: only meaningful in JS/TS source. Closes FPs like | |
| 128 | + # docs/example HTML, playground/self-contained skills that hardcode | |
| 129 | + # innerHTML strings with zero user input (#410). | |
| 130 | + "path_filter": lambda p: p.endswith(_JS_EXTS), | |
| 120 | 131 | "substrings": [".innerHTML =", ".innerHTML="], |
| 121 | 132 | "reminder": "⚠️ Security Warning: Setting innerHTML with untrusted content can lead to XSS vulnerabilities. Use textContent for plain text or safe DOM methods for HTML content. If you need HTML support, consider using an HTML sanitizer library such as DOMPurify.", |
| 122 | 133 | }, |
| @@ -217,11 +228,15 @@ | ||
| 217 | 228 | }, |
| 218 | 229 | { |
| 219 | 230 | "ruleName": "outerHTML_xss", |
| 231 | + # Browser DOM API: only meaningful in JS/TS source. | |
| 232 | + "path_filter": lambda p: p.endswith(_JS_EXTS), | |
| 220 | 233 | "substrings": [".outerHTML =", ".outerHTML="], |
| 221 | 234 | "reminder": "⚠️ Security Warning: Use textContent or sanitize with DOMPurify. outerHTML assignment is an XSS sink equivalent to innerHTML.", |
| 222 | 235 | }, |
| 223 | 236 | { |
| 224 | 237 | "ruleName": "insertAdjacentHTML_xss", |
| 238 | + # Browser DOM API: only meaningful in JS/TS source. | |
| 239 | + "path_filter": lambda p: p.endswith(_JS_EXTS), | |
| 225 | 240 | "substrings": [".insertAdjacentHTML("], |
| 226 | 241 | "reminder": "⚠️ Security Warning: Use insertAdjacentText() or sanitize with DOMPurify. insertAdjacentHTML is an XSS sink.", |
| 227 | 242 | }, |
| @@ -1,0 +1,306 @@ | ||
| 1 | +import os | |
| 2 | +import re | |
| 3 | +import shlex | |
| 4 | +import subprocess | |
| 5 | +import time | |
| 6 | + | |
| 7 | +from gitutil import GIT_CMD, _git_toplevel | |
| 8 | +from session_state import with_locked_state | |
| 9 | + | |
| 10 | + | |
| 11 | +RES_NONE = 0 | |
| 12 | +RES_CWD = 1 | |
| 13 | +RES_COMMAND = 2 | |
| 14 | +RES_SHA_SCAN = 3 | |
| 15 | +RES_TOUCHED_PATHS = 4 | |
| 16 | +RES_HINT = 5 | |
| 17 | + | |
| 18 | +COMMIT_SUBCOMMANDS = {("git", "commit"), ("gt", "create"), ("gt", "modify")} | |
| 19 | +PUSH_SUBCOMMANDS = {("git", "push"), ("gt", "submit")} | |
| 20 | + | |
| 21 | +SCAN_SKIP_DIRS = { | |
| 22 | + "node_modules", ".venv", "venv", "__pycache__", ".tox", "dist", | |
| 23 | + "build", "target", ".cache", ".git", "vendor", "site-packages", | |
| 24 | +} | |
| 25 | +SCAN_MAX_DEPTH = int(os.environ.get("SG_REPO_SCAN_MAX_DEPTH", "3")) | |
| 26 | +SCAN_MAX_REPOS = int(os.environ.get("SG_REPO_SCAN_MAX_REPOS", "64")) | |
| 27 | +SCAN_BUDGET_S = float(os.environ.get("SG_REPO_SCAN_BUDGET_S", "4")) | |
| 28 | + | |
| 29 | +_SEPARATORS = frozenset(";&|()\n") | |
| 30 | +_SHA_RE = re.compile(r"^[0-9a-f]{7,40}$") | |
| 31 | +_ENV_ASSIGN_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*=") | |
| 32 | +_PREFIX_WORDS = frozenset(("env", "time", "exec", "command", "nohup")) | |
| 33 | + | |
| 34 | + | |
| 35 | +def _abs(base, p): | |
| 36 | + try: | |
| 37 | + p = os.path.expanduser(p) | |
| 38 | + if not os.path.isabs(p): | |
| 39 | + p = os.path.join(base or os.getcwd(), p) | |
| 40 | + return os.path.normpath(p) | |
| 41 | + except (TypeError, ValueError, OSError): | |
| 42 | + return None | |
| 43 | + | |
| 44 | + | |
| 45 | +def _is_sep(tok): | |
| 46 | + return bool(tok) and set(tok) <= _SEPARATORS | |
| 47 | + | |
| 48 | + | |
| 49 | +def _tokenize(command): | |
| 50 | + if os.sep == "\\": | |
| 51 | + command = command.replace("\\", "\\\\") | |
| 52 | + try: | |
| 53 | + lex = shlex.shlex(command, posix=True, punctuation_chars=";&|()") | |
| 54 | + lex.whitespace_split = True | |
| 55 | + return list(lex) | |
| 56 | + except ValueError: | |
| 57 | + try: | |
| 58 | + return command.replace("&&", " && ").replace(";", " ; ").split() | |
| 59 | + except Exception: | |
| 60 | + return [] | |
| 61 | + | |
| 62 | + | |
| 63 | +def dirs_from_command(command, cwd, subcommands=None): | |
| 64 | + if not isinstance(command, str) or not command.strip(): | |
| 65 | + return [] | |
| 66 | + tokens = _tokenize(command) | |
| 67 | + out = [] | |
| 68 | + cur = cwd or "" | |
| 69 | + i = 0 | |
| 70 | + n = len(tokens) | |
| 71 | + at_start = True | |
| 72 | + while i < n: | |
| 73 | + t = tokens[i] | |
| 74 | + if _is_sep(t): | |
| 75 | + at_start = True | |
| 76 | + i += 1 | |
| 77 | + continue | |
| 78 | + if at_start and (_ENV_ASSIGN_RE.match(t) or t in _PREFIX_WORDS): | |
| 79 | + i += 1 | |
| 80 | + continue | |
| 81 | + if at_start and t in ("cd", "pushd"): | |
| 82 | + if i + 1 < n and not _is_sep(tokens[i + 1]) and tokens[i + 1] not in ("-",) \ | |
| 83 | + and not tokens[i + 1].startswith("-"): | |
| 84 | + nxt = _abs(cur, tokens[i + 1]) | |
| 85 | + if nxt: | |
| 86 | + cur = nxt | |
| 87 | + i += 2 | |
| 88 | + else: | |
| 89 | + i += 1 | |
| 90 | + at_start = False | |
| 91 | + continue | |
| 92 | + prog = os.path.basename(t) if t else t | |
| 93 | + if at_start and prog in ("git", "gt"): | |
| 94 | + j = i + 1 | |
| 95 | + cdir = cur | |
| 96 | + gdir = None | |
| 97 | + wtree = None | |
| 98 | + if prog == "git": | |
| 99 | + while j < n and not _is_sep(tokens[j]): | |
| 100 | + a = tokens[j] | |
| 101 | + if a == "-C" and j + 1 < n: | |
| 102 | + cdir = _abs(cdir, tokens[j + 1]) or cdir | |
| 103 | + j += 2 | |
| 104 | + continue | |
| 105 | + if a == "-c" and j + 1 < n: | |
| 106 | + j += 2 | |
| 107 | + continue | |
| 108 | + if a.startswith("--git-dir="): | |
| 109 | + gdir = _abs(cdir, a.split("=", 1)[1]) | |
| 110 | + j += 1 | |
| 111 | + continue | |
| 112 | + if a == "--git-dir" and j + 1 < n: | |
| 113 | + gdir = _abs(cdir, tokens[j + 1]) | |
| 114 | + j += 2 | |
| 115 | + continue | |
| 116 | + if a.startswith("--work-tree="): | |
| 117 | + wtree = _abs(cdir, a.split("=", 1)[1]) | |
| 118 | + j += 1 | |
| 119 | + continue | |
| 120 | + if a == "--work-tree" and j + 1 < n: | |
| 121 | + wtree = _abs(cdir, tokens[j + 1]) | |
| 122 | + j += 2 | |
| 123 | + continue | |
| 124 | + if a.startswith("-"): | |
| 125 | + j += 1 | |
| 126 | + continue | |
| 127 | + break | |
| 128 | + sub = tokens[j] if j < n and not _is_sep(tokens[j]) else None | |
| 129 | + if subcommands is None or (prog, sub) in subcommands: | |
| 130 | + cand = wtree | |
| 131 | + if not cand and gdir: | |
| 132 | + cand = os.path.dirname(gdir) if os.path.basename(gdir) == ".git" else gdir | |
| 133 | + if not cand: | |
| 134 | + cand = cdir | |
| 135 | + if cand: | |
| 136 | + out.append(cand) | |
| 137 | + i = j + 1 if j < n else j | |
| 138 | + at_start = False | |
| 139 | + continue | |
| 140 | + at_start = False | |
| 141 | + i += 1 | |
| 142 | + return list(dict.fromkeys(d for d in out if d)) | |
| 143 | + | |
| 144 | + | |
| 145 | +def toplevel_from_command(command, cwd, subcommands=None, cwd_root=None): | |
| 146 | + cwd_abs = _abs(None, cwd) if cwd else None | |
| 147 | + for d in dirs_from_command(command, cwd, subcommands): | |
| 148 | + if cwd_root and d == cwd_abs: | |
| 149 | + return cwd_root | |
| 150 | + try: | |
| 151 | + if os.path.isdir(d): | |
| 152 | + top = _git_toplevel(d) | |
| 153 | + if top: | |
| 154 | + return top | |
| 155 | + except OSError: | |
| 156 | + continue | |
| 157 | + return None | |
| 158 | + | |
| 159 | + | |
| 160 | +def scan_roots(cwd): | |
| 161 | + roots = [] | |
| 162 | + for r in (cwd, os.environ.get("CLAUDE_PROJECT_DIR")): | |
| 163 | + if r and os.path.isdir(r): | |
| 164 | + a = os.path.abspath(r) | |
| 165 | + if a not in roots: | |
| 166 | + roots.append(a) | |
| 167 | + return roots | |
| 168 | + | |
| 169 | + | |
| 170 | +def iter_git_repos(roots, max_depth=None, max_repos=None, deadline=None): | |
| 171 | + max_depth = SCAN_MAX_DEPTH if max_depth is None else max_depth | |
| 172 | + max_repos = SCAN_MAX_REPOS if max_repos is None else max_repos | |
| 173 | + seen = set() | |
| 174 | + for root in roots or []: | |
| 175 | + try: | |
| 176 | + if not root or not os.path.isdir(root): | |
| 177 | + continue | |
| 178 | + root = os.path.abspath(root) | |
| 179 | + except OSError: | |
| 180 | + continue | |
| 181 | + base_depth = root.rstrip(os.sep).count(os.sep) | |
| 182 | + for dirpath, dirnames, filenames in os.walk(root): | |
| 183 | + if deadline is not None and time.monotonic() > deadline: | |
| 184 | + return | |
| 185 | + if ".git" in dirnames or ".git" in filenames: | |
| 186 | + try: | |
| 187 | + key = os.path.realpath(dirpath) | |
| 188 | + except OSError: | |
| 189 | + key = dirpath | |
| 190 | + if key not in seen: | |
| 191 | + seen.add(key) | |
| 192 | + yield dirpath | |
| 193 | + if len(seen) >= max_repos: | |
| 194 | + return | |
| 195 | + depth = dirpath.rstrip(os.sep).count(os.sep) - base_depth | |
| 196 | + if depth >= max_depth: | |
| 197 | + dirnames[:] = [] | |
| 198 | + else: | |
| 199 | + dirnames[:] = [ | |
| 200 | + d for d in dirnames | |
| 201 | + if d not in SCAN_SKIP_DIRS and not d.startswith(".") | |
| 202 | + ] | |
| 203 | + | |
| 204 | + | |
| 205 | +def repo_containing_commit(sha, roots, budget_s=None): | |
| 206 | + if not isinstance(sha, str) or not _SHA_RE.match(sha): | |
| 207 | + return None | |
| 208 | + budget_s = SCAN_BUDGET_S if budget_s is None else budget_s | |
| 209 | + deadline = time.monotonic() + budget_s | |
| 210 | + for repo in iter_git_repos(roots, deadline=deadline): | |
| 211 | + try: | |
| 212 | + r = subprocess.run( | |
| 213 | + [*GIT_CMD, "cat-file", "-e", f"{sha}^{{commit}}"], | |
| 214 | + cwd=repo, capture_output=True, timeout=3, | |
| 215 | + ) | |
| 216 | + except (subprocess.TimeoutExpired, FileNotFoundError, OSError): | |
| 217 | + continue | |
| 218 | + if r.returncode == 0: | |
| 219 | + return _git_toplevel(repo) or repo | |
| 220 | + return None | |
| 221 | + | |
| 222 | + | |
| 223 | +def repos_from_paths(paths, cwd=None, limit=200): | |
| 224 | + counts = {} | |
| 225 | + order = [] | |
| 226 | + cache = {} | |
| 227 | + for p in list(paths or [])[:limit]: | |
| 228 | + if not isinstance(p, str) or not p: | |
| 229 | + continue | |
| 230 | + ap = p if os.path.isabs(p) else _abs(cwd, p) | |
| 231 | + if not ap: | |
| 232 | + continue | |
| 233 | + d = os.path.dirname(ap) | |
| 234 | + while d and not os.path.isdir(d): | |
| 235 | + parent = os.path.dirname(d) | |
| 236 | + if parent == d: | |
| 237 | + break | |
| 238 | + d = parent | |
| 239 | + if not d: | |
| 240 | + continue | |
| 241 | + if d in cache: | |
| 242 | + top = cache[d] | |
| 243 | + else: | |
| 244 | + try: | |
| 245 | + top = _git_toplevel(d) if os.path.isdir(d) else None | |
| 246 | + except OSError: | |
| 247 | + top = None | |
| 248 | + cache[d] = top | |
| 249 | + if top: | |
| 250 | + if top not in counts: | |
| 251 | + order.append(top) | |
| 252 | + counts[top] = counts.get(top, 0) + 1 | |
| 253 | + return sorted(order, key=lambda t: (-counts[t], order.index(t))) | |
| 254 | + | |
| 255 | + | |
| 256 | +def save_repo_hint(session_id, repo_root): | |
| 257 | + if not session_id or not repo_root: | |
| 258 | + return | |
| 259 | + | |
| 260 | + def _save(state): | |
| 261 | + state["repo_root_hint"] = repo_root | |
| 262 | + try: | |
| 263 | + with_locked_state(session_id, _save) | |
| 264 | + except Exception: | |
| 265 | + pass | |
| 266 | + | |
| 267 | + | |
| 268 | +def load_repo_hint(session_id): | |
| 269 | + if not session_id: | |
| 270 | + return None | |
| 271 | + try: | |
| 272 | + hint = with_locked_state(session_id, lambda s: s.get("repo_root_hint")) | |
| 273 | + except Exception: | |
| 274 | + return None | |
| 275 | + if isinstance(hint, str) and hint and os.path.isdir(hint): | |
| 276 | + top = _git_toplevel(hint) | |
| 277 | + if top: | |
| 278 | + return top | |
| 279 | + return None | |
| 280 | + | |
| 281 | + | |
| 282 | +_UNSET = object() | |
| 283 | + | |
| 284 | + | |
| 285 | +def resolve_repo_root(cwd, command=None, subcommands=None, sha=None, | |
| 286 | + touched_paths=None, session_id=None, cwd_root=_UNSET): | |
| 287 | + if cwd_root is _UNSET: | |
| 288 | + cwd_root = _git_toplevel(cwd) if cwd else None | |
| 289 | + if command: | |
| 290 | + top = toplevel_from_command(command, cwd, subcommands, cwd_root) | |
| 291 | + if top and top != cwd_root: | |
| 292 | + return top, RES_COMMAND | |
| 293 | + if cwd_root: | |
| 294 | + return cwd_root, RES_CWD | |
| 295 | + if touched_paths: | |
| 296 | + tops = repos_from_paths(touched_paths, cwd) | |
| 297 | + if tops: | |
| 298 | + return tops[0], RES_TOUCHED_PATHS | |
| 299 | + if sha: | |
| 300 | + top = repo_containing_commit(sha, scan_roots(cwd)) | |
| 301 | + if top: | |
| 302 | + return top, RES_SHA_SCAN | |
| 303 | + hint = load_repo_hint(session_id) | |
| 304 | + if hint: | |
| 305 | + return hint, RES_HINT | |
| 306 | + return None, RES_NONE |
| @@ -46,7 +46,7 @@ | ||
| 46 | 46 | - ENABLE_COMMIT_REVIEW: PostToolUse[Bash] commit security review |
| 47 | 47 | |
| 48 | 48 | Other: |
| 49 | -- SECURITY_REVIEW_MODEL: Model for LLM review (default: claude-opus-4-7) | |
| 49 | +- SECURITY_REVIEW_MODEL: Model for LLM review (default: the newest Opus) | |
| 50 | 50 | - ANTHROPIC_API_KEY: Required for LLM-based reviews |
| 51 | 51 | - ANTHROPIC_AUTH_TOKEN: Alternative to API key — OAuth access token sent as Bearer auth. |
| 52 | 52 | Claude Code passes this automatically for OAuth-authenticated users. |
| @@ -58,6 +58,7 @@ | ||
| 58 | 58 | fcntl = None |
| 59 | 59 | import contextlib |
| 60 | 60 | import glob |
| 61 | +import hashlib | |
| 61 | 62 | import json |
| 62 | 63 | import os |
| 63 | 64 | import random |
| @@ -82,6 +83,7 @@ | ||
| 82 | 83 | PROVENANCE_TAG, PROVENANCE_BANNER, |
| 83 | 84 | _read_plugin_version_int, _PV, _USAGE, _USAGE_LOCK, |
| 84 | 85 | _PRICE_PER_MTOK, _PRICE_DEFAULT, _record_usage, _usage_metrics, |
| 86 | + state_dir as _resolve_state_dir, | |
| 85 | 87 | ) |
| 86 | 88 | import extensibility # noqa: E402 |
| 87 | 89 | from patterns import ( # noqa: E402,F401 |
| @@ -95,7 +97,7 @@ | ||
| 95 | 97 | load_state, save_state, with_locked_state, |
| 96 | 98 | ) |
| 97 | 99 | from gitutil import ( # noqa: E402,F401 |
| 98 | - GIT_CMD, | |
| 100 | + GIT_CMD, apply_safe_git_env, remove_copies_on_termination, | |
| 99 | 101 | _git_rev_parse_head, _find_git_index, _diff_pathspec, _temp_index, |
| 100 | 102 | _git_toplevel, _git_dir, _git_rev_list_range, _git_diff_range, |
| 101 | 103 | _detect_main_branch, _git_reflog_recent_commits, _git_name_only, |
| @@ -111,12 +113,18 @@ | ||
| 111 | 113 | from diffstate import ( # noqa: E402,F401 |
| 112 | 114 | STOP_LOOP_STATE_TTL_SEC, PREVIOUS_FINDINGS_TTL_SEC, |
| 113 | 115 | save_baseline_sha, load_baseline_sha, record_touched_path, |
| 114 | - consume_stop_state, restore_unreviewed_stop_state, | |
| 116 | + consume_stop_state, restore_unreviewed_stop_state, record_reviewed_diff, | |
| 115 | 117 | get_baseline_file_content, capture_git_baseline, |
| 116 | 118 | _REVIEWED_SHAS_BASENAME, _REVIEWED_SHAS_CAP, |
| 117 | 119 | _reviewed_shas_path, _load_reviewed_shas, _append_reviewed_shas, |
| 118 | 120 | UNTRACKED_BASELINE_CAP, _list_untracked, compute_v2_review_set, |
| 119 | 121 | ) |
| 122 | +from reporesolve import ( # noqa: E402,F401 | |
| 123 | + RES_NONE, RES_CWD, RES_COMMAND, RES_SHA_SCAN, RES_TOUCHED_PATHS, RES_HINT, | |
| 124 | + COMMIT_SUBCOMMANDS, PUSH_SUBCOMMANDS, | |
| 125 | + toplevel_from_command, repo_containing_commit, scan_roots, | |
| 126 | + resolve_repo_root, save_repo_hint, load_repo_hint, | |
| 127 | +) | |
| 120 | 128 | import llm # noqa: E402 module ref for reassignable globals (_last_call_claude_http_error etc.) |
| 121 | 129 | from llm import ( # noqa: E402,F401 |
| 122 | 130 | ANTHROPIC_API_KEY, ANTHROPIC_AUTH_TOKEN, HAS_API_CREDENTIALS, |
| @@ -190,7 +198,13 @@ | ||
| 190 | 198 | "response." |
| 191 | 199 | ) |
| 192 | 200 | |
| 193 | -def emit_metrics(metrics, rewake_summary=None): | |
| 201 | +def emit_metrics( | |
| 202 | + metrics, | |
| 203 | + rewake_summary=None, | |
| 204 | + additional_context=None, | |
| 205 | + system_message=None, | |
| 206 | + hook_event_name="PostToolUse", | |
| 207 | +): | |
| 194 | 208 | """ |
| 195 | 209 | Write a SyncHookJSONOutput line to stdout for Claude Code to pick up. |
| 196 | 210 | For asyncRewake (Stop) hooks, CC scans stdout for the first {-prefixed line |
| @@ -213,6 +227,45 @@ | ||
| 213 | 227 | rewakeSummary in hooks.json, shown to the user in the terminal as the |
| 214 | 228 | task-notification one-liner. Must be in the same JSON line as the metrics |
| 215 | 229 | because CC stops scanning stdout after the first {-prefixed line. |
| 230 | + | |
| 231 | + `additional_context` (asyncRewake findings): model-visible guidance text. | |
| 232 | + Delivery channel depends on `hook_event_name` because CC's hook-output | |
| 233 | + contract is NOT symmetric across events: | |
| 234 | + | |
| 235 | + - PostToolUse (commit-review, push-sweep): surfaced via the modern | |
| 236 | + hookSpecificOutput.additionalContext protocol. `PostToolUse` is a | |
| 237 | + member of CC's hookSpecificOutput discriminated union | |
| 238 | + (coreSchemas.ts), so the JSON validates and metrics/rewakeSummary | |
| 239 | + are consumed. See #1375 / #1783 for why this replaced the legacy | |
| 240 | + stderr + exit(2) shape for PostToolUse. | |
| 241 | + | |
| 242 | + - Stop / SubagentStop: there is NO `Stop` member in that union, so | |
| 243 | + emitting hookSpecificOutput{hookEventName:"Stop"} makes the whole | |
| 244 | + line fail isSyncHookJSONOutput validation — which on the asyncRewake | |
| 245 | + path silently drops metrics AND rewakeSummary, and (because the | |
| 246 | + legacy stderr write was removed) leaks the raw JSON to the model as | |
| 247 | + the rewake body. CC's asyncRewake delivery actually reads | |
| 248 | + `stderr || stdout` for the model-visible body and only scans stdout | |
| 249 | + JSON for metrics+rewakeSummary — it never reads additionalContext | |
| 250 | + on this path. So for Stop we use the documented clean pattern: | |
| 251 | + guidance on stderr, valid JSON (metrics + rewakeSummary + | |
| 252 | + top-level decision/reason) on stdout. The top-level decision:"block" | |
| 253 | + + reason also covers the sync-fallback path (single-shot `claude -p`, | |
| 254 | + where asyncRewake degrades to a sync Stop hook that reads | |
| 255 | + decision/reason). See #2159. | |
| 256 | + | |
| 257 | + Empty/None additional_context emits neither channel (back-compat for | |
| 258 | + metrics-only callers). | |
| 259 | + | |
| 260 | + `system_message` (optional, asyncRewake only): user-visible TUI message, | |
| 261 | + distinct from rewakeSummary which is the task-notification one-liner. | |
| 262 | + Use sparingly — the rewakeMessage in hooks.json is the primary user | |
| 263 | + surface; systemMessage adds a per-fire override when the static | |
| 264 | + rewakeMessage isn't specific enough for the finding being shown. | |
| 265 | + | |
| 266 | + `hook_event_name` (used only when additional_context is set): selects the | |
| 267 | + delivery channel above. Defaults to "PostToolUse" (commit-review and | |
| 268 | + push-sweep are the most common callers); handle_stop_hook passes "Stop". | |
| 216 | 269 | """ |
| 217 | 270 | head = {} |
| 218 | 271 | if _PV and "pv" not in metrics: |
| @@ -223,6 +276,26 @@ | ||
| 223 | 276 | out = {"metrics": metrics} |
| 224 | 277 | if rewake_summary: |
| 225 | 278 | out["rewakeSummary"] = rewake_summary |
| 279 | + if additional_context: | |
| 280 | + if hook_event_name in ("Stop", "SubagentStop"): | |
| 281 | + # Stop is NOT in CC's hookSpecificOutput union — emitting it there | |
| 282 | + # fails schema validation and drops metrics+rewakeSummary (#2159). | |
| 283 | + # Clean pattern: guidance on stderr (the asyncRewake body channel, | |
| 284 | + # delivered via `stderr || stdout`), top-level decision/reason for | |
| 285 | + # the sync-fallback path. stdout JSON stays valid so metrics + | |
| 286 | + # rewakeSummary survive. | |
| 287 | + sys.stderr.write(additional_context) | |
| 288 | + sys.stderr.flush() | |
| 289 | + out["decision"] = "block" | |
| 290 | + out["reason"] = additional_context | |
| 291 | + else: | |
| 292 | + # PostToolUse et al. — valid union member; modern protocol. | |
| 293 | + out["hookSpecificOutput"] = { | |
| 294 | + "hookEventName": hook_event_name, | |
| 295 | + "additionalContext": additional_context, | |
| 296 | + } | |
| 297 | + if system_message: | |
| 298 | + out["systemMessage"] = system_message | |
| 226 | 299 | print(json.dumps(out), flush=True) |
| 227 | 300 | |
| 228 | 301 | # ===================================================================== |
| @@ -474,6 +547,16 @@ | ||
| 474 | 547 | # otherwise an untracked-only working tree gets every untracked file |
| 475 | 548 | # reviewed on every turn until something tracked is dirtied. |
| 476 | 549 | untracked_now = _f_ut.result() or {} |
| 550 | + if not sha and not _git_toplevel(cwd): | |
| 551 | + hint = load_repo_hint(session_id) | |
| 552 | + if hint: | |
| 553 | + debug_log(f"UPS: cwd is not a git repo; using repo hint {hint!r}") | |
| 554 | + cwd = hint | |
| 555 | + with _cf.ThreadPoolExecutor(max_workers=2) as _ex: | |
| 556 | + _f_sha = _ex.submit(capture_git_baseline, cwd) | |
| 557 | + _f_ut = _ex.submit(_list_untracked, cwd) | |
| 558 | + sha = _f_sha.result() | |
| 559 | + untracked_now = _f_ut.result() or {} | |
| 477 | 560 | head = _git_rev_parse_head(cwd) |
| 478 | 561 | |
| 479 | 562 | # If the previous turn's Stop hook never ran (user interrupt, follow-up |
| @@ -510,7 +593,11 @@ | ||
| 510 | 593 | elif sha: |
| 511 | 594 | debug_log(f"Captured git baseline: {sha[:12]}") |
| 512 | 595 | else: |
| 513 | - debug_log("Failed to capture git baseline (not a git repo?)") | |
| 596 | + # Show cwd so the next reporter can immediately see when this isn't | |
| 597 | + # actually "not a git repo" but a path-encoding / permissions / git | |
| 598 | + # invocation failure. See #2099. | |
| 599 | + debug_log(f"Failed to capture git baseline (cwd={cwd!r}) — not a git repo, " | |
| 600 | + f"or git invocation failed (check log entries above)") | |
| 514 | 601 | |
| 515 | 602 | sys.exit(0) |
| 516 | 603 | |
| @@ -594,8 +681,30 @@ | ||
| 594 | 681 | # detection — it does NOT tolerate `git -c k=v commit` global options, which |
| 595 | 682 | # keeps this hook aligned with CC's commit attribution on what counts as a |
| 596 | 683 | # commit. |
| 597 | -_GIT_COMMIT_RE = re.compile(r'\bgit\s+commit(?:\s|$)') | |
| 598 | -_GIT_AMEND_RE = re.compile(r'\s--amend\b') | |
| 684 | +# | |
| 685 | +# Also matches `gt create` and `gt modify` — Graphite's stacked-PR wrapper | |
| 686 | +# around git. `gt create` produces a new commit (mapped to git commit | |
| 687 | +# semantics); `gt modify` amends the current commit (mapped to git commit | |
| 688 | +# --amend, also flagged by _GIT_AMEND_RE below). The hooks.json matcher | |
| 689 | +# widening for `gt create:*` / `gt modify:*` / `gt submit:*` ships in the | |
| 690 | +# same change set — without that widening this regex change is dead code | |
| 691 | +# because the hook subprocess never spawns for gt invocations. See #2048. | |
| 692 | +_GIT_COMMIT_RE = re.compile( | |
| 693 | + # `git -C <path>` and `git -c key=val` global options are allowed between | |
| 694 | + # `git` and `commit` (mirrors the long-standing tolerance in | |
| 695 | + # _GIT_PUSH_RE). Without this, `git -C /repo commit` is silently dropped | |
| 696 | + # by the handler — see #2089's secondary finding. The gt branch has no | |
| 697 | + # global-option layer to worry about. | |
| 698 | + r'\bgit(?:\s+-[Cc]\s+(?:"[^"]*"|\'[^\']*\'|[^\s"\']\S*)|\s+--[^\s=]+=\S+' | |
| 699 | + r'|\s+--(?:git-dir|work-tree)\s+\S+)*\s+commit\b' | |
| 700 | + r'|\bgt\s+(?:create|modify)\b' | |
| 701 | +) | |
| 702 | +# Match either the `--amend` flag (with the leading whitespace boundary | |
| 703 | +# preserved from the original) OR `gt modify` which is semantically an | |
| 704 | +# amend. The handler treats matches as "find the pre-amend SHA via reflog | |
| 705 | +# and diff against THAT, not against the post-amend HEAD's parent" — same | |
| 706 | +# code path for both git --amend and gt modify. | |
| 707 | +_GIT_AMEND_RE = re.compile(r'(?:\s--amend\b|\bgt\s+modify\b)') | |
| 599 | 708 | |
| 600 | 709 | # Rolling-window cap on LLM commit-review calls. See atomic_check_rate_limit |
| 601 | 710 | # docstring for the rationale that motivated the switch from a lifetime cap. |
| @@ -624,8 +733,14 @@ | ||
| 624 | 733 | # entry would buy minimal extra coverage (sessions that push only via gh) at |
| 625 | 734 | # the cost of an extra python spawn on every `... && gh pr create` compound |
| 626 | 735 | # (the common case). Those sessions are caught on their next standalone `git push`. |
| 736 | +# Matches `git push` (with optional `-c k=v` / `-C path` global options | |
| 737 | +# CC's hooks.json matcher doesn't tolerate) OR `gt submit` — Graphite's | |
| 738 | +# stacked-PR push command. gt submit forwards to `git push` internally, | |
| 739 | +# but the bash hook fires on Claude's top-level command so we need to | |
| 740 | +# recognize gt submit at the matcher level. See #2048. | |
| 627 | 741 | _GIT_PUSH_RE = re.compile( |
| 628 | - r'\bgit(?:\s+-[cC]\s+\S+|\s+--\S+=\S+)*\s+push\b' | |
| 742 | + r'(?:\bgit(?:\s+-[cC]\s+(?:"[^"]*"|\'[^\']*\'|[^\s"\']\S*)|\s+--[^\s=]+=\S+' | |
| 743 | + r'|\s+--(?:git-dir|work-tree)\s+\S+)*\s+push\b|\bgt\s+submit\b)' | |
| 629 | 744 | ) |
| 630 | 745 | |
| 631 | 746 | # `git push` stdout: "abc1234..def5678 branch -> branch" (or `+abc..def` on |
| @@ -663,7 +778,12 @@ | ||
| 663 | 778 | cwd = input_data.get("cwd") |
| 664 | 779 | if not tuid or not cwd: |
| 665 | 780 | return True |
| 666 | - gd = _git_dir(_git_toplevel(cwd) or cwd) | |
| 781 | + command = (input_data.get("tool_input") or {}).get("command", "") or "" | |
| 782 | + gd = _git_dir( | |
| 783 | + _git_toplevel(cwd) | |
| 784 | + or toplevel_from_command(command, cwd, COMMIT_SUBCOMMANDS | PUSH_SUBCOMMANDS) | |
| 785 | + or cwd | |
| 786 | + ) | |
| 667 | 787 | if not gd: |
| 668 | 788 | return True |
| 669 | 789 | # GC: best-effort sweep of stale sentinels so they don't accumulate. |
| @@ -791,23 +911,30 @@ | ||
| 791 | 911 | # @{u}@{1} — only meaningful if an upstream is configured. |
| 792 | 912 | for ref in ("@{u}@{1}", "@{push}@{1}"): |
| 793 | 913 | try: |
| 914 | + # See #2099: stdout is a SHA but stderr can carry non-ASCII git | |
| 915 | + # warnings — keep bytes raw to avoid cp1252 reader-thread crash. | |
| 794 | 916 | r = subprocess.run( |
| 795 | 917 | [*GIT_CMD, "rev-parse", "--verify", "-q", ref], |
| 796 | - cwd=repo_root, capture_output=True, text=True, timeout=5, | |
| 918 | + cwd=repo_root, capture_output=True, timeout=5, | |
| 797 | 919 | ) |
| 798 | - if r.returncode == 0 and r.stdout.strip(): | |
| 799 | - return r.stdout.strip() | |
| 920 | + sha = r.stdout.decode("utf-8", errors="replace").strip() | |
| 921 | + if r.returncode == 0 and sha: | |
| 922 | + return sha | |
| 800 | 923 | except (subprocess.TimeoutExpired, FileNotFoundError, OSError): |
| 801 | 924 | pass |
| 802 | 925 | main = _detect_main_branch(repo_root) |
| 803 | 926 | if main: |
| 804 | 927 | try: |
| 928 | + # See #2099: drop text=True; decode bytes manually so a | |
| 929 | + # cp1252-undefined byte in git's stderr doesn't crash the | |
| 930 | + # reader thread. | |
| 805 | 931 | r = subprocess.run( |
| 806 | 932 | [*GIT_CMD, "merge-base", "HEAD", main], |
| 807 | - cwd=repo_root, capture_output=True, text=True, timeout=5, | |
| 933 | + cwd=repo_root, capture_output=True, timeout=5, | |
| 808 | 934 | ) |
| 809 | - if r.returncode == 0 and r.stdout.strip(): | |
| 810 | - return r.stdout.strip() | |
| 935 | + sha = r.stdout.decode("utf-8", errors="replace").strip() | |
| 936 | + if r.returncode == 0 and sha: | |
| 937 | + return sha | |
| 811 | 938 | except (subprocess.TimeoutExpired, FileNotFoundError, OSError): |
| 812 | 939 | pass |
| 813 | 940 | return None |
| @@ -938,9 +1065,10 @@ | ||
| 938 | 1065 | # emitting labels like `[pre-commit abc1234]`, and on (b) chained |
| 939 | 1066 | # `git commit || git log --stat` where `N files changed` appears in output |
| 940 | 1067 | # even though the commit itself failed. |
| 1068 | + all_shas = _COMMIT_SHA_RE.findall(bash_output) | |
| 941 | 1069 | commit_succeeded = ( |
| 942 | 1070 | not interrupted |
| 943 | - and _COMMIT_SHA_RE.search(bash_output) is not None | |
| 1071 | + and bool(all_shas) | |
| 944 | 1072 | and any(p.search(bash_output) for p in _COMMIT_DIFFSTAT_PATTERNS) |
| 945 | 1073 | ) |
| 946 | 1074 | |
| @@ -948,6 +1076,18 @@ | ||
| 948 | 1076 | # commit_review and group by commit_review_on. |
| 949 | 1077 | _base = {"commit_review": True, "commit_review_on": COMMIT_REVIEW_ENABLED} |
| 950 | 1078 | |
| 1079 | + cwd_root = _git_toplevel(cwd) if cwd else None | |
| 1080 | + repo_root = cwd_root | |
| 1081 | + repo_res = RES_CWD if cwd_root else RES_NONE | |
| 1082 | + if cwd: | |
| 1083 | + _cmd_root = toplevel_from_command(command, cwd, COMMIT_SUBCOMMANDS, cwd_root) | |
| 1084 | + if _cmd_root and _cmd_root != cwd_root: | |
| 1085 | + repo_root, repo_res = _cmd_root, RES_COMMAND | |
| 1086 | + if not cwd_root: | |
| 1087 | + _base["cwd_is_repo"] = False | |
| 1088 | + if repo_res != RES_CWD: | |
| 1089 | + _base["repo_resolution"] = repo_res | |
| 1090 | + | |
| 951 | 1091 | # Reflog fallback for hidden stdout. Analysis of skip_reason=21 emissions |
| 952 | 1092 | # showed a large share were commits that DID succeed |
| 953 | 1093 | # but whose `[branch sha]` line was hidden by piping/redirection/-q |
| @@ -965,7 +1105,12 @@ | ||
| 965 | 1105 | _reflog_shas: List[str] = [] |
| 966 | 1106 | _skip_21_sub = 0 |
| 967 | 1107 | if not commit_succeeded and not interrupted and cwd: |
| 968 | - _root = _git_toplevel(cwd) | |
| 1108 | + if not repo_root: | |
| 1109 | + repo_root = load_repo_hint(session_id) | |
| 1110 | + if repo_root: | |
| 1111 | + repo_res = RES_HINT | |
| 1112 | + _base["repo_resolution"] = repo_res | |
| 1113 | + _root = repo_root | |
| 969 | 1114 | _fresh, _stale = _git_reflog_recent_commits(_root) |
| 970 | 1115 | if _fresh: |
| 971 | 1116 | _already = _load_reviewed_shas(_root) |
| @@ -1016,11 +1161,24 @@ | ||
| 1016 | 1161 | emit_metrics({"skipped": True, "skip_reason": 25, **_base}) |
| 1017 | 1162 | sys.exit(0) |
| 1018 | 1163 | |
| 1019 | - repo_root = _git_toplevel(cwd) | |
| 1164 | + if not repo_root and all_shas and not _reflog_shas: | |
| 1165 | + repo_root = repo_containing_commit(all_shas[-1], scan_roots(cwd)) | |
| 1166 | + if repo_root: | |
| 1167 | + repo_res = RES_SHA_SCAN | |
| 1020 | 1168 | if not repo_root: |
| 1169 | + repo_root = load_repo_hint(session_id) | |
| 1170 | + if repo_root: | |
| 1171 | + repo_res = RES_HINT | |
| 1172 | + if repo_res != RES_CWD: | |
| 1173 | + _base["repo_resolution"] = repo_res | |
| 1174 | + if not repo_root: | |
| 1021 | 1175 | debug_log("Commit review: not in a git repo") |
| 1022 | 1176 | emit_metrics({"skipped": True, "skip_reason": 26, **_base}) |
| 1023 | 1177 | sys.exit(0) |
| 1178 | + if repo_res != RES_CWD: | |
| 1179 | + debug_log(f"Commit review: repo resolved via {repo_res} -> {repo_root!r}") | |
| 1180 | + if repo_res in (RES_COMMAND, RES_TOUCHED_PATHS): | |
| 1181 | + save_repo_hint(session_id, repo_root) | |
| 1024 | 1182 | |
| 1025 | 1183 | # Pin the review to the exact SHA the Bash command produced, parsed from |
| 1026 | 1184 | # its stdout. Reviewing HEAD instead is wrong when the commit was made in |
| @@ -1047,7 +1205,6 @@ | ||
| 1047 | 1205 | # all are reviewed. |
| 1048 | 1206 | shas = _reflog_shas |
| 1049 | 1207 | else: |
| 1050 | - all_shas = _COMMIT_SHA_RE.findall(bash_output) | |
| 1051 | 1208 | shas = [all_shas[-1]] if all_shas else [] |
| 1052 | 1209 | if not shas: |
| 1053 | 1210 | debug_log("Commit review: no SHA in commit output") |
| @@ -1118,16 +1275,21 @@ | ||
| 1118 | 1275 | resolved = 0 |
| 1119 | 1276 | for sha in shas: |
| 1120 | 1277 | try: |
| 1278 | + # core.quotePath=false: emit raw UTF-8 in `diff --git a/... b/...` | |
| 1279 | + # headers so non-ASCII paths aren't C-quoted past the downstream | |
| 1280 | + # parse_diff_into_files regex (sibling of #2056 / #2075). See #2082. | |
| 1281 | + # core.quotePath=false comes from GIT_CMD globally (see gitutil.py). | |
| 1121 | 1282 | if pre_amend_sha: |
| 1122 | 1283 | # Delta review: pre-amend → post-amend. `git diff` (not show) |
| 1123 | 1284 | # so the output is a pure unified diff with no commit header. |
| 1124 | 1285 | result = subprocess.run( |
| 1125 | - [*GIT_CMD, "diff", "--no-color", "--no-ext-diff", pre_amend_sha, sha, "--"], | |
| 1286 | + [*GIT_CMD, "diff", "--no-color", "--no-ext-diff", "--no-textconv", | |
| 1287 | + pre_amend_sha, sha, "--"], | |
| 1126 | 1288 | cwd=repo_root, capture_output=True, timeout=15 |
| 1127 | 1289 | ) |
| 1128 | 1290 | else: |
| 1129 | 1291 | result = subprocess.run( |
| 1130 | - [*GIT_CMD, "show", "-p", "--no-color", "--no-ext-diff", sha, "--"], | |
| 1292 | + [*GIT_CMD, "show", "-p", "--no-color", "--no-ext-diff", "--no-textconv", sha, "--"], | |
| 1131 | 1293 | cwd=repo_root, capture_output=True, timeout=15 |
| 1132 | 1294 | ) |
| 1133 | 1295 | except (subprocess.TimeoutExpired, FileNotFoundError, OSError) as e: |
| @@ -1254,12 +1416,13 @@ | ||
| 1254 | 1416 | try: |
| 1255 | 1417 | full_shas = [] |
| 1256 | 1418 | for s in shas: |
| 1419 | + # See #2099: drop text=True; decode manually for cp1252 safety. | |
| 1257 | 1420 | r = subprocess.run( |
| 1258 | 1421 | [*GIT_CMD, "rev-parse", "--verify", "-q", s], |
| 1259 | - cwd=repo_root, capture_output=True, text=True, timeout=5, | |
| 1422 | + cwd=repo_root, capture_output=True, timeout=5, | |
| 1260 | 1423 | ) |
| 1261 | 1424 | if r.returncode == 0: |
| 1262 | - full_shas.append(r.stdout.strip()) | |
| 1425 | + full_shas.append(r.stdout.decode("utf-8", errors="replace").strip()) | |
| 1263 | 1426 | _append_reviewed_shas(repo_root, full_shas, vulns_found=len(vulns or [])) |
| 1264 | 1427 | except Exception: |
| 1265 | 1428 | pass |
| @@ -1361,18 +1524,26 @@ | ||
| 1361 | 1524 | if s in sev: |
| 1362 | 1525 | sev[s] += 1 |
| 1363 | 1526 | |
| 1527 | + # Rebuild guidance from new_vulns only — concrete_guidance from the LLM | |
| 1528 | + # still lists deduped entries. Pass via additional_context so CC surfaces | |
| 1529 | + # the reason via hookSpecificOutput.additionalContext instead of empty | |
| 1530 | + # stdout (#1783) / stderr-only "json output validation failed" (#1375). | |
| 1531 | + _commit_guidance = (PROVENANCE_BANNER + "\n\n" | |
| 1532 | + + _format_vulns_guidance(new_vulns) | |
| 1533 | + + CONTINUATION_SUFFIX + "\n") | |
| 1364 | 1534 | emit_metrics({ |
| 1365 | 1535 | "vulns_found": len(new_vulns), **_base, **_agentic_m, |
| 1366 | 1536 | "critical_count": sev["critical"], "high_count": sev["high"], |
| 1367 | 1537 | "files_reviewed": len(diff_files), "review_ms": review_ms, |
| 1368 | 1538 | **({"deduped": n_deduped} if n_deduped else {}), |
| 1369 | - }, rewake_summary=_format_vulns_summary(new_vulns, prefix="Commit security review found")) | |
| 1539 | + }, rewake_summary=_format_vulns_summary(new_vulns, prefix="Commit security review found"), | |
| 1540 | + additional_context=_commit_guidance, | |
| 1541 | + hook_event_name="PostToolUse") | |
| 1370 | 1542 | |
| 1371 | - # Rebuild guidance from new_vulns only — concrete_guidance from the LLM | |
| 1372 | - # still lists deduped entries. | |
| 1373 | - sys.stderr.write(PROVENANCE_BANNER + "\n\n" | |
| 1374 | - + _format_vulns_guidance(new_vulns) | |
| 1375 | - + CONTINUATION_SUFFIX + "\n") | |
| 1543 | + # exit(2) is preserved per the asyncRewake protocol — it's what CC | |
| 1544 | + # uses as the "force fix" signal that triggers the rewakeMessage flow. | |
| 1545 | + # The stderr.write was removed; additional_context above now carries | |
| 1546 | + # the same text via the modern JSON channel. See #1358/#1375/#1783. | |
| 1376 | 1547 | sys.exit(2) |
| 1377 | 1548 | |
| 1378 | 1549 | def handle_push_sweep_posttooluse(input_data): |
| @@ -1426,10 +1597,24 @@ | ||
| 1426 | 1597 | if not cwd: |
| 1427 | 1598 | emit_metrics({"skipped": True, "skip_reason": 25, **_base}) |
| 1428 | 1599 | sys.exit(0) |
| 1429 | - repo_root = _git_toplevel(cwd) | |
| 1600 | + _tip_m = _PUSH_RANGE_RE.search(_push_section(bash_output)) | |
| 1601 | + cwd_root = _git_toplevel(cwd) | |
| 1602 | + repo_root, repo_res = resolve_repo_root( | |
| 1603 | + cwd, command, PUSH_SUBCOMMANDS, | |
| 1604 | + sha=_tip_m.group(2) if _tip_m else None, | |
| 1605 | + session_id=session_id, cwd_root=cwd_root, | |
| 1606 | + ) | |
| 1607 | + if not cwd_root: | |
| 1608 | + _base["cwd_is_repo"] = False | |
| 1609 | + if repo_res != RES_CWD: | |
| 1610 | + _base["repo_resolution"] = repo_res | |
| 1430 | 1611 | if not repo_root: |
| 1431 | 1612 | emit_metrics({"skipped": True, "skip_reason": 26, **_base}) |
| 1432 | 1613 | sys.exit(0) |
| 1614 | + if repo_res != RES_CWD: | |
| 1615 | + debug_log(f"Push sweep: repo resolved via {repo_res} -> {repo_root!r}") | |
| 1616 | + if repo_res in (RES_COMMAND, RES_TOUCHED_PATHS): | |
| 1617 | + save_repo_hint(session_id, repo_root) | |
| 1433 | 1618 | |
| 1434 | 1619 | # Guard: the sweep diffs `base..HEAD` and the agent Reads the working |
| 1435 | 1620 | # tree, so the pushed ref MUST be HEAD or the review is of the wrong |
| @@ -1453,9 +1638,10 @@ | ||
| 1453 | 1638 | # both. |
| 1454 | 1639 | head = None |
| 1455 | 1640 | try: |
| 1641 | + # See #2099: drop text=True; decode manually for cp1252 safety. | |
| 1456 | 1642 | r = subprocess.run([*GIT_CMD, "rev-parse", "HEAD"], cwd=repo_root, |
| 1457 | - capture_output=True, text=True, timeout=5) | |
| 1458 | - head = r.stdout.strip() if r.returncode == 0 else None | |
| 1643 | + capture_output=True, timeout=5) | |
| 1644 | + head = r.stdout.decode("utf-8", errors="replace").strip() if r.returncode == 0 else None | |
| 1459 | 1645 | except (subprocess.TimeoutExpired, FileNotFoundError, OSError): |
| 1460 | 1646 | pass |
| 1461 | 1647 | push_section = _push_section(bash_output or "") |
| @@ -1485,14 +1671,15 @@ | ||
| 1485 | 1671 | quiet_success = False |
| 1486 | 1672 | if not (bash_output or "").strip() and not interrupted: |
| 1487 | 1673 | try: |
| 1674 | + # See #2099: drop text=True; decode manually for cp1252 safety. | |
| 1488 | 1675 | r_cur = subprocess.run( |
| 1489 | 1676 | [*GIT_CMD, "rev-parse", "--verify", "-q", "@{u}"], |
| 1490 | - cwd=repo_root, capture_output=True, text=True, timeout=5) | |
| 1677 | + cwd=repo_root, capture_output=True, timeout=5) | |
| 1491 | 1678 | r_prev = subprocess.run( |
| 1492 | 1679 | [*GIT_CMD, "rev-parse", "--verify", "-q", "@{u}@{1}"], |
| 1493 | - cwd=repo_root, capture_output=True, text=True, timeout=5) | |
| 1494 | - cur = r_cur.stdout.strip() if r_cur.returncode == 0 else "" | |
| 1495 | - prev_u = r_prev.stdout.strip() if r_prev.returncode == 0 else "" | |
| 1680 | + cwd=repo_root, capture_output=True, timeout=5) | |
| 1681 | + cur = r_cur.stdout.decode("utf-8", errors="replace").strip() if r_cur.returncode == 0 else "" | |
| 1682 | + prev_u = r_prev.stdout.decode("utf-8", errors="replace").strip() if r_prev.returncode == 0 else "" | |
| 1496 | 1683 | quiet_success = bool(cur and prev_u and cur == head and prev_u != cur) |
| 1497 | 1684 | except (subprocess.TimeoutExpired, FileNotFoundError, OSError): |
| 1498 | 1685 | pass |
| @@ -1506,11 +1693,12 @@ | ||
| 1506 | 1693 | # reviewed-shas state. |
| 1507 | 1694 | for local_ref in new_branch_matches: |
| 1508 | 1695 | try: |
| 1696 | + # See #2099: drop text=True; decode manually for cp1252 safety. | |
| 1509 | 1697 | r = subprocess.run( |
| 1510 | 1698 | [*GIT_CMD, "rev-parse", "--verify", "-q", local_ref], |
| 1511 | - cwd=repo_root, capture_output=True, text=True, timeout=5, | |
| 1699 | + cwd=repo_root, capture_output=True, timeout=5, | |
| 1512 | 1700 | ) |
| 1513 | - local_sha = r.stdout.strip() if r.returncode == 0 else "" | |
| 1701 | + local_sha = r.stdout.decode("utf-8", errors="replace").strip() if r.returncode == 0 else "" | |
| 1514 | 1702 | except (subprocess.TimeoutExpired, FileNotFoundError, OSError): |
| 1515 | 1703 | local_sha = "" |
| 1516 | 1704 | if local_sha and local_sha != head: |
| @@ -1629,17 +1817,23 @@ | ||
| 1629 | 1817 | # Metrics — keep within the 10-key cap; agentic sub-metrics are dropped |
| 1630 | 1818 | # here in favour of the push-sweep funnel keys (telemetry can join on session_id |
| 1631 | 1819 | # to the per-commit fires for agentic detail). rewake_summary must ride |
| 1632 | - # this line (CC reads only the first {-prefixed stdout line); it's a | |
| 1633 | - # no-op when new_vulns is empty since we exit 0 below. | |
| 1634 | - emit_metrics({ | |
| 1820 | + # this line (CC reads only the first {-prefixed stdout line); the emit | |
| 1821 | + # is deferred to the two exit points below so the with-vulns path can | |
| 1822 | + # also pass additional_context in the same JSON line (#1375/#1783) — | |
| 1823 | + # the by-design "CC keeps only the first JSON line" constraint means | |
| 1824 | + # we can't emit twice. Builds the shared metrics dict here; vulns path | |
| 1825 | + # adds additional_context, no-vulns path emits as-is. | |
| 1826 | + _push_metrics = { | |
| 1635 | 1827 | **_base, "pushed": len(push_range), "unreviewed": len(tail), |
| 1636 | 1828 | "prefix_advanced": prefix_advanced, "vulns_found": len(new_vulns), |
| 1637 | 1829 | "files_reviewed": len(diff_files), "review_ms": review_ms, |
| 1638 | 1830 | **({"deduped": n_deduped} if n_deduped else {}), |
| 1639 | - }, rewake_summary=_format_vulns_summary(new_vulns, prefix="Push security review found")) | |
| 1831 | + } | |
| 1832 | + _push_rewake_summary = _format_vulns_summary(new_vulns, prefix="Push security review found") | |
| 1640 | 1833 | |
| 1641 | 1834 | if not new_vulns: |
| 1642 | 1835 | debug_log("Push sweep: no new findings") |
| 1836 | + emit_metrics(_push_metrics, rewake_summary=_push_rewake_summary) | |
| 1643 | 1837 | sys.exit(0) |
| 1644 | 1838 | |
| 1645 | 1839 | # First-push of a big branch can surface many findings at once across |
| @@ -1692,9 +1886,14 @@ | ||
| 1692 | 1886 | guidance = _format_vulns_guidance(reported) or "" |
| 1693 | 1887 | else: |
| 1694 | 1888 | guidance = concrete_guidance or _format_vulns_guidance(reported) or "" |
| 1695 | - sys.stderr.write( | |
| 1696 | - PROVENANCE_BANNER + "\n\n" + guidance + CONTINUATION_SUFFIX + "\n" | |
| 1697 | - ) | |
| 1889 | + # Emit metrics + additional_context together — single JSON line is the | |
| 1890 | + # contract CC's hook parser expects. exit(2) preserved as the asyncRewake | |
| 1891 | + # "force fix" trigger (see comment near handle_commit_review_posttooluse). | |
| 1892 | + # See #1358 / #1375 / #1783. | |
| 1893 | + emit_metrics(_push_metrics, rewake_summary=_push_rewake_summary, | |
| 1894 | + additional_context=(PROVENANCE_BANNER + "\n\n" | |
| 1895 | + + guidance + CONTINUATION_SUFFIX + "\n"), | |
| 1896 | + hook_event_name="PostToolUse") | |
| 1698 | 1897 | sys.exit(2) |
| 1699 | 1898 | |
| 1700 | 1899 | def handle_stop_hook(input_data): |
| @@ -1711,6 +1910,8 @@ | ||
| 1711 | 1910 | session_id = input_data.get("session_id", "default") |
| 1712 | 1911 | stop_hook_active = input_data.get("stop_hook_active", False) |
| 1713 | 1912 | cwd = input_data.get("cwd", "") |
| 1913 | + hook_event_name = input_data.get("hook_event_name") or "Stop" | |
| 1914 | + is_subagent = hook_event_name == "SubagentStop" | |
| 1714 | 1915 | |
| 1715 | 1916 | # Recursion guard FIRST — consume_stop_state clears touched_paths, and CC |
| 1716 | 1917 | # sets stop_hook_active session-wide while any asyncRewake Stop is in |
| @@ -1725,7 +1926,8 @@ | ||
| 1725 | 1926 | # git, network). asyncRewake Stop runs in the background; the next turn's |
| 1726 | 1927 | # UPS/PostToolUse can fire while we're still here. The snapshot is immune |
| 1727 | 1928 | # to those writes — they affect the NEXT Stop fire's snapshot. |
| 1728 | - snap = consume_stop_state(session_id) | |
| 1929 | + snap = (consume_stop_state(session_id, clear=False) if is_subagent | |
| 1930 | + else consume_stop_state(session_id)) | |
| 1729 | 1931 | fire_count = snap["fire_count"] |
| 1730 | 1932 | touched_paths = snap["touched_paths"] |
| 1731 | 1933 | baseline_sha = snap["baseline_sha"] |
| @@ -1789,9 +1991,27 @@ | ||
| 1789 | 1991 | debug_log("Stop hook: no cwd") |
| 1790 | 1992 | _skip(4) |
| 1791 | 1993 | |
| 1994 | + repo_cwd, repo_res = resolve_repo_root( | |
| 1995 | + cwd, touched_paths=touched_paths, session_id=session_id) | |
| 1996 | + res_metrics = ({} if repo_res == RES_CWD | |
| 1997 | + else {"cwd_is_repo": False, "repo_resolution": repo_res}) | |
| 1998 | + if is_subagent and repo_cwd: | |
| 1999 | + _pd = os.environ.get("CLAUDE_PROJECT_DIR") | |
| 2000 | + _pd_root = _git_toplevel(_pd) if _pd and os.path.isdir(_pd) else None | |
| 2001 | + if _pd_root and _pd_root != repo_cwd: | |
| 2002 | + debug_log(f"Stop hook: SubagentStop in {repo_cwd!r}, session repo is {_pd_root!r}") | |
| 2003 | + v2_metrics = dict(res_metrics) | |
| 2004 | + _skip(11) | |
| 2005 | + if repo_res != RES_CWD and repo_cwd: | |
| 2006 | + debug_log(f"Stop hook: repo resolved via {repo_res} -> {repo_cwd!r}") | |
| 2007 | + if not is_subagent and repo_res in (RES_COMMAND, RES_TOUCHED_PATHS): | |
| 2008 | + save_repo_hint(session_id, repo_cwd) | |
| 2009 | + cwd = repo_cwd | |
| 2010 | + | |
| 1792 | 2011 | review_paths, diff_base, repo_root, untracked, v2_metrics = compute_v2_review_set( |
| 1793 | 2012 | cwd, baseline_sha, head_at_capture, untracked_at_baseline |
| 1794 | 2013 | ) |
| 2014 | + v2_metrics = {**res_metrics, **v2_metrics} | |
| 1795 | 2015 | if not review_paths: |
| 1796 | 2016 | debug_log("Stop hook: empty review set") |
| 1797 | 2017 | _skip(9, touched_paths_count=len(touched_paths)) |
| @@ -1820,6 +2040,11 @@ | ||
| 1820 | 2040 | debug_log("Stop hook: no changes since baseline") |
| 1821 | 2041 | _skip(6) |
| 1822 | 2042 | |
| 2043 | + diff_hash = hashlib.sha256(diff_output.encode("utf-8", "replace")).hexdigest() | |
| 2044 | + if snap.get("reviewed_diff_hash") == diff_hash: | |
| 2045 | + debug_log("Stop hook: diff already reviewed by SubagentStop") | |
| 2046 | + _skip(12) | |
| 2047 | + | |
| 1823 | 2048 | # Parse diff into per-file content |
| 1824 | 2049 | diff_files = parse_diff_into_files(diff_output) |
| 1825 | 2050 | if not diff_files: |
| @@ -1890,12 +2115,15 @@ | ||
| 1890 | 2115 | for v in vulns |
| 1891 | 2116 | ] |
| 1892 | 2117 | # Update baseline so next stop hook iteration only sees new changes |
| 1893 | - new_sha = capture_git_baseline(cwd) | |
| 2118 | + new_sha = None if is_subagent else capture_git_baseline(cwd) | |
| 1894 | 2119 | new_untracked_baseline = _list_untracked(cwd) if new_sha else None |
| 1895 | 2120 | |
| 1896 | 2121 | def _record_fire(state): |
| 1897 | - state["stop_hook_fire_count"] = fire_index | |
| 1898 | - state["stop_hook_fire_count_ts"] = _time.time() | |
| 2122 | + if is_subagent: | |
| 2123 | + state["reviewed_diff_hash"] = diff_hash | |
| 2124 | + else: | |
| 2125 | + state["stop_hook_fire_count"] = fire_index | |
| 2126 | + state["stop_hook_fire_count_ts"] = _time.time() | |
| 1899 | 2127 | # Re-read under lock — the commit-review PostToolUse hook may have |
| 1900 | 2128 | # appended findings since consume_stop_state snapshotted. |
| 1901 | 2129 | # Dedupe on (filePath, category) — vulnerableCode includes diff |
| @@ -1927,6 +2155,11 @@ | ||
| 1927 | 2155 | # untracked_baseline_n is the signal for whether the UPS-time |
| 1928 | 2156 | # untracked-snapshot capture actually ran. |
| 1929 | 2157 | sweep_trimmed = {k: v for k, v in sweep.items() if k != "warn_unresolved_mask"} |
| 2158 | + # Pass guidance via additional_context so CC surfaces the findings via | |
| 2159 | + # hookSpecificOutput.additionalContext instead of stderr-only (which | |
| 2160 | + # was the cause of "json output validation failed" / empty-reason UI in | |
| 2161 | + # #1375 / #1783). exit(2) preserved as the asyncRewake "force fix" | |
| 2162 | + # signal — that's the documented mechanism. See #1358 / #1375 / #1783. | |
| 1930 | 2163 | emit_metrics({ |
| 1931 | 2164 | "vulns_found": len(vulns), |
| 1932 | 2165 | "untracked_baseline_n": len(untracked_at_baseline), |
| @@ -1939,11 +2172,12 @@ | ||
| 1939 | 2172 | "fire_index": fire_index, |
| 1940 | 2173 | **({"diff_truncated": llm._last_review_truncated_bytes} |
| 1941 | 2174 | if llm._last_review_truncated_bytes else {}), |
| 2175 | + **({"repo_resolution": repo_res} if res_metrics else {}), | |
| 1942 | 2176 | **sweep_trimmed, |
| 1943 | - }, rewake_summary=_format_vulns_summary(vulns)) | |
| 1944 | - | |
| 1945 | - # Exit code 2 with stderr forces Claude to continue and fix | |
| 1946 | - sys.stderr.write(PROVENANCE_BANNER + "\n\n" + concrete_guidance + CONTINUATION_SUFFIX + "\n") | |
| 2177 | + }, rewake_summary=_format_vulns_summary(vulns), | |
| 2178 | + additional_context=(PROVENANCE_BANNER + "\n\n" | |
| 2179 | + + concrete_guidance + CONTINUATION_SUFFIX + "\n"), | |
| 2180 | + hook_event_name=hook_event_name) | |
| 1947 | 2181 | sys.exit(2) |
| 1948 | 2182 | |
| 1949 | 2183 | if llm._last_call_claude_http_error is not None: |
Diff not shown: this commit changes too much to display at once.
Diff not shown: this commit changes too much to display at once.
Diff not shown: this commit changes too much to display at once.
Diff not shown: this commit changes too much to display at once.
Diff not shown: this commit changes too much to display at once.
Diff not shown: this commit changes too much to display at once.
Diff not shown: this commit changes too much to display at once.
Diff not shown: this commit changes too much to display at once.