Skip to content

feat(review): add a security-focused review profile toggle - #2675

Merged
JSONbored merged 1 commit into
mainfrom
feat/security-focused-review-profile
Jul 3, 2026
Merged

feat(review): add a security-focused review profile toggle#2675
JSONbored merged 1 commit into
mainfrom
feat/security-focused-review-profile

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • review.profile (src/signals/focus-manifest.ts) only ever tuned HOW MANY findings the AI reviewer surfaces (chill suppresses nits, assertive raises them) — there was no way to tell the reviewer WHAT KIND of defect to prioritize. Adds a new, orthogonal review.security_focus: boolean toggle (mirroring the existing review.inline_comments boolean pattern rather than extending the chill/balanced/assertive union, since this is a "what to hunt for" axis, not a fourth volume level).
  • Parses review.security_focus in parseReviewConfig, adds it to FocusManifestReviewConfig, reviewConfigToJson (cache round-trip), and resolveReviewPromptOverrides — the same manifest-only sites review.profile and review.inline_comments already flow through (no DB column / OpenAPI field needed, confirmed via npm run ui:openapi:settings-parityreview.profile itself isn't in RepositorySettingsSchema either, it's a .gittensory.yml-only field).
  • Wires it through src/services/ai-review.ts: a new SECURITY_FOCUS_SUFFIX constant appended in buildSystemPrompt right after the existing REVIEW_PROFILE_SUFFIX, naming injection, authn/authz bypass, secret handling, unsafe deserialization, SSRF, and path traversal as elevated-scrutiny categories. It composes with (never replaces) the profile suffix.
  • Threads securityFocus through src/queue/processors.ts (resolveReviewPromptOverrides destructure → runAiReviewForAdvisory args → runGittensoryAiReview call) and adds it to src/review/ai-review-cache-input.ts's fingerprint payload so flipping the toggle invalidates the AI-review cache instead of replaying a stale review.
  • Off/unset by default at every layer, so an existing repo's reviewer prompt is byte-identical.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format, for example fix(api): restore profile access checks.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked an issue, or this is small enough that the summary explains why an issue is not needed.

Validation

  • git diff --check
  • npm run actionlint (via npm run test:ci)
  • npm run typecheck
  • npm run test:coverage locally; codecov/patch requires ≥99% coverage of the lines AND branches you changed (aim for 100% on your diff so CI variance does not fail near the threshold). Global coverage is a non-blocking trend with a loose 90% backstop, not the gate.
  • npm run test:workers
  • npm run build:mcp
  • npm run test:mcp-pack
  • npm run ui:openapi:check
  • npm run ui:lint
  • npm run ui:typecheck
  • npm run ui:build
  • npm audit --audit-level=moderate
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries

All run via the full local gate: npm run test:ci (actionlint, migrations check, cf-typegen check, typecheck, unsharded test:coverage, test:workers, build:mcp, test:mcp-pack, build:miner, rees:test, ui:openapi:check, ui:openapi:settings-parity, ui:version-audit, ui:lint, ui:typecheck, ui:test, ui:build) — all green, 6740+ tests passing — plus npm audit --audit-level=moderate reporting 0 vulnerabilities. Added/updated tests: test/unit/focus-manifest.test.ts (parsing, round-trip, resolveReviewPromptOverrides composition), test/unit/ai-review.test.ts (system-prompt composition with and without profile), test/unit/ai-review-cache-input.test.ts (fingerprint changes when the toggle flips, independent of profile), plus fixture updates in test/unit/ai-review-cache.test.ts, test/unit/queue.test.ts, and test/unit/signals-coverage.test.ts for the new required field.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests.
  • API/OpenAPI/MCP behavior is updated and tested where needed.
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks.
  • Visible UI changes include a UI Evidence section below with JPG/JPEG or PNG screenshots arranged as organized, captioned, clickable thumbnails. SVG screenshots are not used as review evidence. Review-only screenshots or recordings are not committed to the repository.
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs.

No auth/cookie/CORS/session/UI/docs changes in this PR — it's a backend prompt-composition + manifest-parsing change only, so those boxes don't apply.

UI Evidence

Not applicable — no UI/frontend/docs changes in this PR.

Notes

  • Left the actual review.security_focus field out of RepositorySettingsSchema/OpenAPI intentionally: review.profile, review.inline_comments, and review.path_instructions are all manifest-only (.gittensory.yml-only) fields with no DB column, confirmed unaffected by npm run ui:openapi:settings-parity (still reports the same 62 fields) — security_focus follows that exact precedent rather than the full DB+OpenAPI config-as-code checklist, which applies to RepositorySettings-backed fields.

Closes #2566

review.profile's chill/balanced/assertive only tune how many findings
surface, never what kind the reviewer prioritizes. Add an orthogonal
review.security_focus boolean (mirroring the review.inline_comments
toggle pattern) that appends an explicit system-prompt instruction to
hunt injection, authn/authz bypass, secret handling, unsafe
deserialization, SSRF, and path-traversal patterns with elevated
scrutiny. It composes with (never replaces) the profile suffix and is
threaded through the AI-review cache fingerprint so a toggle flip
forces a fresh review. Off/unset by default, so existing repos see a
byte-identical reviewer prompt.

Closes #2566
@dosubot dosubot Bot added the size:M label Jul 3, 2026
@codecov

codecov Bot commented Jul 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.10%. Comparing base (642b12d) to head (38a7f78).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2675   +/-   ##
=======================================
  Coverage   96.10%   96.10%           
=======================================
  Files         237      237           
  Lines       26538    26542    +4     
  Branches     9624     9626    +2     
=======================================
+ Hits        25505    25509    +4     
  Misses        424      424           
  Partials      609      609           
Files with missing lines Coverage Δ
src/queue/processors.ts 92.57% <ø> (ø)
src/review/ai-review-cache-input.ts 100.00% <ø> (ø)
src/services/ai-review.ts 95.27% <100.00%> (+0.02%) ⬆️
src/signals/focus-manifest.ts 99.29% <100.00%> (+<0.01%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JSONbored JSONbored self-assigned this Jul 3, 2026
@JSONbored
JSONbored merged commit c52a69c into main Jul 3, 2026
11 checks passed
@JSONbored
JSONbored deleted the feat/security-focused-review-profile branch July 3, 2026 05:16
JSONbored added a commit that referenced this pull request Jul 3, 2026
…boot (#2686)

* fix(review): add the missing securityFocus field to 5 AI-review-cache test fixtures

#2675 (feat(review): add a security-focused review profile toggle) added
securityFocus as a required AiReviewCacheInput field after these fixtures
(from #2667) were written, breaking npm run typecheck on main for anyone
branching fresh off it.

* fix(selfhost): reject known-placeholder and weak critical secrets at boot

.env.selfhost.example shipped ENABLED (not commented-out) placeholder values
for GITHUB_WEBHOOK_SECRET, GITTENSORY_API_TOKEN, GITTENSORY_MCP_TOKEN,
INTERNAL_JOB_TOKEN, and SELFHOST_SETUP_TOKEN. An operator who copies the
starter to .env per the quickstart docs and misses "fill in the placeholders"
runs an instance with a PUBLICLY KNOWN webhook HMAC secret (forgeable
signatures) and PUBLICLY KNOWN static bearer tokens -- GITTENSORY_API_TOKEN
authenticates as the server-to-server actor and bypasses per-repo write
checks, INTERNAL_JOB_TOKEN gates internal routes -- silently, with no error
at boot or runtime.

- The boot-time preflight check (already gates server.ts's main(), throwing
  before the process starts serving) now rejects any of the five critical
  secrets that is set to the exact known-placeholder string, or that is
  merely too short to be a real generated secret, or that duplicates another
  critical secret's value. Presence is still each secret's own concern (most
  are feature-gating, not universally required) -- this only judges
  STRENGTH whenever one is actually set, so it can never be silently bypassed
  by leaving the file's placeholder in place.
- .env.selfhost.example now ships these five lines commented out, with
  explicit per-secret generation guidance, instead of enabled placeholders.
- The quickstart doc callout it directed users through now explicitly warns
  about generating distinct random values for each secret.

Defense in depth: the docs + example file guide an operator toward doing the
right thing, and the preflight check makes doing the wrong thing impossible
rather than merely discouraged.
JSONbored added a commit that referenced this pull request Jul 3, 2026
… test fixtures (#2684)

#2675 (feat(review): add a security-focused review profile toggle) added
securityFocus as a required AiReviewCacheInput field after these fixtures
(from #2667) were written, breaking npm run typecheck on main for anyone
branching fresh off it.
JSONbored added a commit that referenced this pull request Jul 3, 2026
countOpenItemsForAuthorAcrossRepos (merged via #2678, closing #2562)
counted an author's open PRs/issues across the ENTIRE D1 database with
no installation scoping at all -- on a database shared by multiple
installations (the hosted product's normal shape, and possible on
self-host too), a contributor's activity on one installation could
wrongly trigger GLOBAL_CONTRIBUTOR_OPEN_ITEM_CAP closes on a completely
unrelated installation that never gated them.

Scope the query through repositories.installationId first (matching
the existing markRepositoriesRemovedFromInstallation precedent), then
inArray(...) against the resulting repoFullNames -- this codebase has
no Drizzle joins to lean on instead.

Also fixes 5 AiReviewCacheInput test fixtures in queue.test.ts left
broken by an unrelated already-merged PR (#2675, security-focused
review profile) that added a required securityFocus field without
updating these fixtures -- main's typecheck was red without this,
which this PR's own CI would otherwise have inherited.
JSONbored added a commit that referenced this pull request Jul 3, 2026
…ion (#2687)

* fix(agent-actions): scope the global open-item cap query by installation

countOpenItemsForAuthorAcrossRepos (merged via #2678, closing #2562)
counted an author's open PRs/issues across the ENTIRE D1 database with
no installation scoping at all -- on a database shared by multiple
installations (the hosted product's normal shape, and possible on
self-host too), a contributor's activity on one installation could
wrongly trigger GLOBAL_CONTRIBUTOR_OPEN_ITEM_CAP closes on a completely
unrelated installation that never gated them.

Scope the query through repositories.installationId first (matching
the existing markRepositoriesRemovedFromInstallation precedent), then
inArray(...) against the resulting repoFullNames -- this codebase has
no Drizzle joins to lean on instead.

Also fixes 5 AiReviewCacheInput test fixtures in queue.test.ts left
broken by an unrelated already-merged PR (#2675, security-focused
review profile) that added a required securityFocus field without
updating these fixtures -- main's typecheck was red without this,
which this PR's own CI would otherwise have inherited.

* fix(agent-actions): audit a truncated installation repo list, don't drop it silently

listRepoFullNamesForInstallation's .limit(20_000) meant an installation
with more tracked repos than that would silently undercount toward
GLOBAL_CONTRIBUTOR_OPEN_ITEM_CAP with no signal anything was dropped.
Records an audit event on the rare install where the limit is still
hit, mirroring the same observability pattern already used for the
per-author item-count truncation in this file.

Addresses a gate review finding on #2687.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(review): add a security-focused review profile

1 participant