GitDesktop / Tickets / #94 feat(ai,pulls,automations,activity): add dedicated security models and review runtimes

Anonymous - 2026-07-20

Originally posted by: cloudflare-workers-and-pages[bot]

Deploying gitdesktop with Cloudflare Pages

Latest commit: 6e90231

Status: ✅ Deploy successful!

Preview URL: https://4ebe5f4e.gitdesktop.pages.dev

Branch Preview URL: https://feat-review-runtime-security.gitdesktop.pages.dev

View logs

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-20

assigned_to: theBGuy
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-20

Originally posted by: theBGuy

Notes for reviewers

Recorded decisions + evidence for this batch (two features: optional dedicated security-audit model, and review runtime display). Numbered so findings can reference them.

Batch shape. 3-package delegated wave (settings/runner/panel · store/ticker/dock/history · panel runtime follow-up) + one whole-feature adversarial spec review: per-package verdicts approve/approve/approve, zero blocking findings. pnpm build green; all scoped biome checks clean; live E2E on the running app (item 12).

securityReviewAi absence semantics. Deliberately NOT in DEFAULT_SETTINGS; loadSettings nested-merges only when present; toggle-OFF saves undefined, which JSON serialization drops. Live-verified on disk both directions: ON+Save → key present; OFF+Save → key absent. Discard after toggling ON reverts to absent (live-verified).

Only the security path branches. effectiveReviewAi(settings, mode) returns the security config for mode === "security" only. AI conflict resolution and CI-debug deliberately keep reviewAi (rule of three — per-mode models for those wait for real demand; please don't suggest generalizing now).

Panel precedence (user-confirmed design). Untouched inline picker → each Run button uses its own mode's configured model; an explicit in-panel pick is a per-run override that wins for BOTH buttons. The hint under the picker names the security model when it differs — comparison is provider+model only (a config differing only in cliPath/base-URL shows no hint; deliberate, those aren't the user-facing distinguishers).

Key/CLI warnings key off the picker's provider only (accepted design). A security config on a keyless provider surfaces as a normal run error rather than a pre-run warning; the hint names the security provider, which is the affordance for noticing the mismatch.

Runtime stamp semantics. startedAt is stamped at the RUNNING transition — queue wait is deliberately excluded from elapsed. The persisted history record's startedAt moved to the same stamp, so records saved before this change measured from enqueue: historical durations mix two semantics (noted in a code comment in ReviewHistory). Old records with zero span show no duration by design (the > 0 guard).

Ticker architecture. ElapsedTime clones relative-time.tsx's module-level shared-snapshot pattern at 1s (useSyncExternalStore; interval starts at 0→1 subscribers with an immediate refresh, cleared at 1→0). No bare Date.now() in any render path — this repo has a documented React Compiler freeze gotcha; please don't suggest per-row setInterval or render-time clock reads.

Per-second re-renders are confined to mounted ElapsedTime spans. Store entries carry static stamps; nothing patches the store per tick, so useReviewTasks consumers are untouched by ticking.

Stopped-row duration requires BOTH stamps. A run cancelled while still queued has no startedAt and shows no "ran X" (deliberate — it never ran).

No error-state duration in the panel. "took X" renders on done only; failed/cancelled durations live on the dock's stopped rows. Deliberate scope.

Disclosed orchestrator fix (live-E2E-caught): the automation runner's persistReviewHistory previously wrote startedAt: now, finishedAt: now — automation-delivered reviews could never show a history duration (that's why pre-batch records have none). Now threads the run's real start (runStartedMs, captured at registerAutomationRun). Not yet exercised by a live automation delivery — the first automated review on this PR is the confirmation.

Live E2E evidence (dev app, scratch-repo PR): security audit ran sonnet while the picker showed opus (history row "Security · sonnet · 31s"); panel elapsed ticked 4s→26s beside Cancel; dock row showed "Security audit · Preparing review… · 27s" in lockstep with the panel (shared snapshot); "took 31s" on the finished result; settings round-trips per item 2. NOT live-tested: the stopped-row "ran X" (needs an automation cancel; statically verified) and item 11's fix.

formatDuration contract: "42s" under a minute (floored), "3m 12s" under an hour (seconds unpadded), "1h 2m" above; non-finite/negative clamp to "0s". Edge cases walked: 59_999→"59s", 60_000→"1m 0s", 3_599_999→"59m 59s", 3_600_000→"1h 0m".

Settings toggle seeds from the current DRAFT review config ({ ...reviewAi } at toggle-ON), not the persisted one — an unsaved review-model edit carries into the seed, which matches the batched Save/Discard form model.

Docs-sync decisions. README AI-review bullet + in-app help guide updated (security-model copy in the ai-gated section; dock/runtime copy in the activity section); changelog fragments for both features; the marketing site deliberately skipped — a model knob and a runtime readout are below the capability-line bar. Recorded here so the skip is a decision, not an omission.

Posted by GitDesktop — automated agent comment, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-20

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

I've verified the store lifecycle, the runner wiring, the settings merge, the panel precedence logic, and the shared ticker against the actual source. Here's my assessment.

Summary

This adds an optional dedicated securityReviewAi config for security audits (falling back to reviewAi) plus live/total run-duration display across the panel, dock, and history. The logic is sound and the state modeling is careful — start/end stamps are cleared on re-run, the queue wait is correctly excluded, the shared ticker mounts/unmounts its interval cleanly, and effectiveReviewAi is threaded consistently through the runner (lane pick, generation, comment model, persisted history). No blockers. Two should-fixes below.

Correctness / UX

should-fix — PrReviewPanel.tsx, needsKey/keyPreview vs. the Security-audit button. The key warning is computed only for the visible provider (provider = (reviewOverride ?? globalReviewAi)?.provider), never for securityReviewAi. Concrete case: review model = Anthropic (key saved), dedicated security model = OpenAI with no OpenAI key saved, no in-panel override. The panel shows no warning (Anthropic has a key), but clicking Security audit routes to securityReviewAi and the run fails into phase === "error" with the generic "The review failed. Check the model and try again." — instead of the specific "No OpenAI API key saved" hint the user would get if OpenAI were the general provider. Same gap applies to the CLI not-found / not-signed-in warnings. Suggested fix: when !reviewOverride && securityReviewAi would be used, also surface a key/availability warning for securityReviewAi.provider (e.g. compute a second needsKey/preview for it), so the dedicated security model's readiness is visible before the button is clicked.

Docs / conventions

should-fix — marketing site not updated. The diff updates README.md, src/features/help/content.ts, and adds two changelog.d/ fragments, but not site/src/pages/index.astro. The repo's "keep docs in sync — every time, unprompted" rule lists the marketing capabilities/FeatureRow as step 2, and the author clearly did the other three surfaces (so this reads as an omission rather than a deliberate "too minor" call, which the notes don't record). Confirm whether the site's AI-review copy needs the dedicated-security-model / runtime mention, or note it as a deliberate skip since these refine an existing capability.

Notes acknowledged (not re-flagged)

The securityReviewAi absence semantics (not in DEFAULT_SETTINGS, nested-merge only when present, undefined on toggle-off) are an explicit recorded decision (note [#2]) and the loadSettings merge in the diff implements exactly that; a stray null also degrades safely via effectiveReviewAi's ?? and the panel's truthiness check.

Per-mode branching limited to "security" (note [#3]) matches effectiveReviewAi.

The mixed historical durations in ReviewHistory (old records stamped startedAt at enqueue) are called out inline in the diff and guarded by the finishedAt - startedAt > 0 check, which also naturally hides the old automation records that stamped startedAt === finishedAt.

Posted by GitDesktop — AI output, verify before acting on it.

Related

Tickets: #2
Tickets: #3
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-20

Originally posted by: theBGuy

🤖 GitDesktop AI security audit · opus · automated

The changes are self-contained: display of elapsed timers and a new optional securityReviewAi config that flows through the exact same generateReviewText / provider plumbing as the existing reviewAi. Adding a parallel config object that reuses identical code paths introduces no new trust boundary — any host-allowlist / guarded-fetch enforcement that applied to reviewAi applies identically to securityReviewAi.

No security issues found in these changes.

The AI config (securityReviewAi) is the user's own local settings, not attacker-controlled; the Ollama/OpenAI-compatible URLs are user-configurable connections (a non-issue).

Model names/provider labels rendered in PrReviewPanel, ReviewHistory, and ActivityDock are React-escaped and come from trusted config.

formatDuration, the shared ticker in elapsed-time.tsx, and the timestamp plumbing in reviews.ts/runner.ts are pure display/timing logic with no injection sink.

Embedding diff/PR content into review prompts is the product working as designed, and the security-model selection only changes which model runs — it doesn't let untrusted data alter a security decision or escalate tool/privilege access.

Posted by GitDesktop — AI output, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-20

Originally posted by: theBGuy

Round 1 dispositions

AI review should-fix 1 (security-model readiness warnings) — accepted, with the overturn acknowledged. This finding fairly overturns notes item 5's recorded carve-out ("key/CLI warnings key off the picker's provider only"): the concrete case — review model keyed, security model on a keyless provider, no override — strands the user in a generic run error when the panel already owns the exact warning machinery. Fix in the next push: when the Security-audit button would use the dedicated config (!reviewOverride && securityReviewAi, same condition as the hint) and its provider differs from the picker's, the panel surfaces the same three readiness warnings (missing API key / CLI not found / CLI not signed in) computed for securityReviewAi.provider, phrased as security-audit warnings. Hooks stay unconditional (second secret-preview + detect query, render-gated), and when both configs share a provider the existing warnings already cover it, so nothing doubles up.

AI review should-fix 2 (marketing site) — declined; the deliberate-skip record exists. Notes item 15 (posted before the reviews fired) records precisely the call this finding asks to see: site deliberately skipped, "recorded here so the skip is a decision, not an omission." Both features refine the existing AI-review capability line rather than adding a capability — the repo's own bar for a site entry. (The review's notes-acknowledged section suggests the lift may have truncated before item 15; it's in the PR conversation in full.) Same disposition on Copilot's README:411 thread.

Copilot content.ts:1626 ({{ai}} gating) — accepted and fixed (one-line: the {{ai}} opener moved before the elapsed-timer sentence; every review run is an AI feature, so the timer sentence is AI-only content). In the next push.

Bonus evidence for notes item 11: this PR's own round-1 automated reviews are the live confirmation the runner duration fix needed — the persisted history records carry real spans now (general 255.1s, security 52.0s, sequential), where every pre-batch automation record had startedAt === finishedAt. Item 11's "not yet exercised" caveat is closed.

Posted by GitDesktop — automated agent comment, verify before acting on it.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-20

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

Verified against source. My assessment is complete.

This adds an optional dedicated securityReviewAi config (falling back to reviewAi via effectiveReviewAi) plus live/total run-duration display across the panel, dock, and history. The logic is sound — the fallback is centralized and threaded consistently through the runner, the start/end stamps clear on re-run and exclude queue wait, and the shared ticker mounts/unmounts cleanly. No blockers. One narrow should-fix and one nit.

Resolved since last review

Security-model readiness warnings (prev should-fix 1) — resolved. The current PrReviewPanel.tsx computes securityWarnApplies and renders the three security-specific readiness warnings (missing key / CLI not found / CLI not signed in) keyed off securityReviewAi.provider, so a keyless dedicated security provider no longer strands the user in the generic error state.

Elapsed-timer sentence outside the AI gate in help/content.ts (Copilot + prev context) — resolved. The {{ai}} opener now precedes "A review still running…", so the timer copy is hidden under Hide AI.

Marketing site not updated (prev should-fix 2 / Copilot) — not re-flagged; recorded as a deliberate skip in the author's notes (item 15: both features refine the existing AI-review capability line, below the site-entry bar).

Correctness / UX

should-fix — PrReviewPanel.tsx, securityWarnApplies (line 202) vs. the general cliDetect (line 176). The security readiness warnings gate on securityReviewAi?.provider !== provider, so they are entirely skipped when the security config shares the general provider. But the general cliDetect runs detectAgentCli(cliKind!, reviewAi?.cliPath) against the picker's cliPath, not the security config's. Concrete case: general review = claude with a working cliPath, dedicated security = claude with a custom cliPath that points at a missing/renamed binary, no in-panel override. securityWarnApplies is false (providers match), secCliDetect is disabled, and the general detect (working path) shows no warning — so clicking Security audit dies into the generic "The review failed" state, exactly what these warnings were added to prevent. Suggested fix: also run/surface the CLI readiness check when the providers match but securityReviewAi.cliPath !== reviewAi.cliPath (rendering only the CLI-not-found / not-signed-in lines in that sub-case, since a shared provider means a shared key so the key warning stays redundant).

Readability

nit — PrReviewPanel.tsx, secKeyPreview (line 208). Unlike secCliDetect, useSecretPreview(secProvider) has no enabled gate, so when no security config exists it still fetches the secret preview for the default "anthropic". Harmless (presence-only, and deduped when it matches the picker's provider), but the inline comment claims the hooks stay "cheap when the security path doesn't apply" — which holds for the detect query but not this one. Consider aligning the two or trimming the comment's claim.

Posted by GitDesktop — AI output, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-20

Originally posted by: theBGuy

Round 2 dispositions

should-fix (same-provider / different-cliPath escapes the readiness gate) — accepted and fixed in the next push. Verified real: securityWarnApplies required provider inequality, so a security config sharing the general provider but pointing at a broken custom CLI path rendered nothing and died generic — an emergent gap in round 1's own fix, fair catch. The gate is now split: providerDiffers (all three warnings, key included) vs cliPathDiffers (same provider, different path → the two CLI warnings only — same provider means same key, per the finding's own sub-case reasoning; the two gates are mutually exclusive by construction). secCliDetect's enabled rides providerDiffers || cliPathDiffers, and the detect query was already keyed on the security config's own cliPath, so the probe tests the right binary.

nit (secKeyPreview ungated vs the "cheap" comment claim) — accepted, aligned rather than trimmed. Now useSecretPreview(providerDiffers ? secProvider : provider): when the security path is inactive or shares the provider, the argument equals the picker's own call and dedupes to the same cache entry — zero extra fetch. The dedupe claim was verified against the hook's source before relying on it: settingsKeys.secret(provider) is purely provider-keyed (["secret-present", provider]), so identical arguments provably share one entry. .data is only read under providerDiffers, where the argument is the security provider.

For the record — why no security-audit round 2: this setup's automations config enables the security audit on pr-open only; pr-sync runs the general review alone. The round-1 audit (clean) covered the full diff at ready-flip; push rounds are general-only by configuration, not by malfunction — the per-mode watermark machinery was verified live (real headSha on both modes' round-1 records).

Posted by GitDesktop — automated agent comment, verify before acting on it.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-20

Originally posted by: theBGuy

🤖 GitDesktop AI review · opus · automated

This adds an optional dedicated securityReviewAi config (with a centralized effectiveReviewAi fallback to reviewAi) plus live/total run-duration display across the panel, dock, and history. The logic is sound and consistent across the manual (PrReviewPanel.run) and automated (runner.run → effectiveReviewAi) paths, the start/end stamps clear on re-run and exclude queue wait, the shared ticker mounts/unmounts cleanly, and the round-2 readiness gaps are now closed. No blockers, and nothing I'd hold the merge for.

Resolved since last review

Same-provider / different-cliPath escapes the readiness gate (prev round-2 should-fix) — resolved in the current diff. PrReviewPanel.tsx now splits the gate into providerDiffers (all three warnings) and cliPathDiffers (CLI warnings only, since a shared provider shares its key). secCliDetect.enabled rides providerDiffers || cliPathDiffers, and the query is keyed on securityReviewAi?.cliPath, so the probe tests the security config's own binary. The concrete stranding case (general claude on a working path, security claude on a broken custom path, no override) now surfaces the CLI-not-found / not-signed-in lines.

secKeyPreview ungated vs. the "cheap" comment (prev nit) — resolved. It's now useSecretPreview(providerDiffers ? secProvider : provider); when the security path is inactive or shares the provider, the argument equals the picker's own call and dedupes to the same provider-keyed cache entry (.data is only read under providerDiffers). Verified secProvider only flows into the fetch under providerDiffers.

Triage of other tools' findings

Copilot help/content.ts:1626 (elapsed-timer sentence outside the AI gate) — resolved in the current diff: the {{ai}} opener now precedes "A review still running… shows a live elapsed timer", so the copy is hidden under Hide AI.

Copilot README.md:411 (marketing site not updated) — the author recorded this as a deliberate skip (notes item 15: both features refine the existing AI-review capability line, below the site-entry bar). Acknowledged, not re-flagged.

The rest checks out: effectiveReviewAi and the panel's run() resolve the same effective config (override → security config for mode === "security" → globalReviewAi), the runner threads reviewCfg uniformly through lane pick / generation / comment label / persisted model, the store clears startedAt/endedAt on re-run and stamps startedAt only after acquireSlot (queue wait excluded), and formatDuration clamps non-finite/negative input to "0s". ElapsedTime's ticker uses stable module-level subscribe/getNow, refreshes on the 0→1 transition, and clears on 1→0 — no leak and StrictMode-safe.

Posted by GitDesktop — AI output, verify before acting on it.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2026-07-20

Ticket changed by: theBGuy

status: open --> closed
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

feat(ai,pulls,automations,activity): add dedicated security models and...

Milestone

Searches

Help

#94 feat(ai,pulls,automations,activity): add dedicated security models and review runtimes

Security-audit model selection

Review runtime tracking

Discussion

Deploying gitdesktop with Cloudflare Pages

Notes for reviewers

Summary

Correctness / UX

Docs / conventions

Notes acknowledged (not re-flagged)

Related

Round 1 dispositions

Resolved since last review

Correctness / UX

Readability

Round 2 dispositions

Resolved since last review

Triage of other tools' findings

Latest commit:	`6e90231`
Status:	✅ Deploy successful!
Preview URL:	https://4ebe5f4e.gitdesktop.pages.dev
Branch Preview URL:	https://feat-review-runtime-security.gitdesktop.pages.dev