feat(github-app): reviewer-routing auto_request action - #833
Conversation
|
Note Gittensory Gate skippedPR closed before full evaluation. No late first comment was created.
💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers. |
This comment has been minimized.
This comment has been minimized.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #833 +/- ##
========================================
Coverage 96.71% 96.71%
========================================
Files 108 110 +2
Lines 14641 14749 +108
Branches 5298 5333 +35
========================================
+ Hits 14160 14265 +105
- Misses 102 103 +1
- Partials 379 381 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| .map((seg) => | ||
| seg | ||
| .split("*") | ||
| .map((s) => s.replace(/[.+^${}()|[\]\\]/g, "\\$&")) |
There was a problem hiding this comment.
P2: ReDoS vulnerability in CODEOWNERS glob-to-regex conversion
CODEOWNERS glob-to-regex conversion does not escape ?, enabling ReDoS via malicious patterns.
Add ? to the escaped regex metacharacters in matchesCodeownersPattern or use a dedicated glob-to-regex library.
AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.
<file name="src/signals/reviewer-routing.ts">
<violation number="1" location="src/signals/reviewer-routing.ts:73">
<priority>P2</priority>
<title>ReDoS vulnerability in CODEOWNERS glob-to-regex conversion</title>
<evidence>The matchesCodeownersPattern function converts CODEOWNERS glob patterns into regular expressions by escaping some regex metacharacters, but the ? character is not escaped. A repository owner or attacker who can modify a target repository's CODEOWNERS file could inject patterns containing many ? characters. These become regex quantifiers (?) that can trigger catastrophic backtracking when matched against file paths, causing CPU exhaustion and Denial of Service in the gittensory worker processing the pull request.</evidence>
<recommendation>Escape the ? character alongside other regex metacharacters in the replace call within matchesCodeownersPattern. Update the regex from /[.+^${}()|[\]\]/g to /[.+^${}()|[\]\?]/g so that ? is treated as a literal character in CODEOWNERS patterns, eliminating the ReDoS vector.</recommendation>
</violation>
</file>
The first Wave-2 / Phase-0 piece: a graduated autonomy dial the whole agent layer reads before acting. Deny-by-default. - AutonomyLevel (observe -> suggest -> propose -> auto_with_approval -> auto) + AgentActionClass (review/request_changes/approve/merge/close/ label) + AutonomyPolicy (per-action-class map) types. - src/settings/autonomy.ts: resolveAutonomy(autonomy, actionClass) — THE single gate the action layer (#778) consults; returns 'observe' for any unset/malformed class (deny-by-default). Plus isActingAutonomyLevel, autonomyRequiresApproval, normalizeAutonomyPolicy. Pure + 100% covered. - Persisted on repository_settings as a JSON map (migration 0042, default '{}' = deny-by-default), mirroring commandAuthorization; parsed + resolved through the existing yml > DB > defaults resolver. Resolvable from .gittensory.yml via the settings: block (a malformed block never blanks the DB-configured policy). Surfaced in the GET /settings OpenAPI shape. The richer autoMaintain config block (merge/close policy, requireApprovals) + dashboard write are #774; the action layer that consults resolveAutonomy is #778. NOTE: migration 0042 is also used by the open #833 (reviewer-routing) — whichever merges second rebases + renumbers.
Builds on #773: the per-repo auto-maintain policy + a dashboard surface. - AutoMaintainPolicy { requireApprovals, mergeMethod } + normalizeAuto- MaintainPolicy (conservative defaults squash/1; requireApprovals clamped to [0,10]; invalid merge method -> squash). Pure + 100% covered. - Persisted on repository_settings as JSON (migration 0043, default '{}' -> defaults), mirroring autonomy/commandAuthorization. - Config-as-code: .gittensory.yml settings.autoMaintain (+ settings. autonomy from #773) resolved through the existing yml > DB > defaults resolver; a non-mapping block is ignored, never blanking the DB policy. - Dashboard: a new 'Auto-maintain (agent layer)' section in the maintainer settings editor (#130) — per-action autonomy selectors for all six action classes + approvals-before-auto-merge + merge method. - Maintainer PUT /settings now accepts autonomy + autoMaintain (bounded: requireApprovals 0..10; unknown autonomy action classes dropped by the DB normalizer; out-of-range approvals rejected at the boundary). NOTE: migration 0043 follows #773's 0042 — both ahead of the open #833; whichever merges into a 0042/0043 collision rebases + renumbers. The action layer that consults this (resolveAutonomy + the policy) is #778.
Phase-0 safety controls the action layer (#778) must consult before any action — the second gate alongside resolveAutonomy. - src/settings/agent-execution.ts (pure, 100% covered): - resolveAgentActionMode -> paused | dry_run | live. Safest wins: a global OR per-repo pause halts everything; else dry-run logs without mutating; else live. agentActionModeExecutes is true only for live. - isGlobalAgentPause: the operator emergency brake via env AGENT_ACTIONS_PAUSED (truthy-string idiom). - buildAgentActionAudit: a structured who/what/why/outcome/mode audit record (eventType agent.action.<class>) so live actions AND dry-run shadows record on one shape — extends the existing audit-event infra. - Per-repo settings agentPaused + agentDryRun (migration 0044, default false), wired like badgeEnabled across types/schema/repositories/openapi /settings-preview yml block, plus the maintainer PUT /settings. - Dashboard: kill-switch + dry-run toggles in the auto-maintain section. Deferred to #778 (needs real actions): the action layer honoring the mode, the dry-run feed into the recommendation-outcome loop, and revert-where- possible. NOTE: migration 0044 follows #773/#774's 0042/0043, ahead of the open #833.
JSONbored
left a comment
There was a problem hiding this comment.
Good:
- Feature is directionally useful for maintainer automation.
- CODEOWNERS parsing and reviewer request helpers have meaningful tests.
- Settings/API surface is mostly threaded through.
Bad:
- Merge state is DIRTY with real conflicts against current main.
- Adds migrations/0042_reviewer_routing_mode.sql while main already has 0042_agent_autonomy.sql.
- getRequestedReviewers returns empty set on API failure even though caller comment says this should skip conservatively.
- CODEOWNERS fetch uses public raw HEAD, not installation-auth/base/ref-pinned content.
- Codecov fails and Superagent flagged one security concern.
Change requests:
- Rebase/renumber migration and regenerate conflicted OpenAPI artifacts.
- Make requested-reviewer lookup failure skip auto-request, not proceed as “none requested.”
- Fetch CODEOWNERS from the correct repo/ref using installation auth where possible.
- Resolve Codecov/security flag before merge.
|
@JSONbored Could you plz review my PR? Thanks! |
JSONbored
left a comment
There was a problem hiding this comment.
PR #833 — feat(github-app): reviewer-routing auto_request action [YB0y]
Action: CHANGES REQUESTED
Issue #830: PARTIALLY CI: pass
Good:
- Clean CODEOWNERS parser/ranker + installation-auth reviewer-request helpers; strict opt-in +
newcomer guard; migration renumbered 0042→0046 (the earlier collision is resolved). Meaningful tests.
Flagged:
- Unresolved ReDoS review thread (reviewer-routing.ts ~73): glob→regex does not escape
?— still
absent from the escaped metacharacter set; attacker-controlled CODEOWNERS can trigger it. - getRequestedReviewers returns an empty Set on API failure, so the caller proceeds as "none
requested" (can re-request) — contradicts its own comment; should fail-closed/skip. - CODEOWNERS fetched from public raw HEAD, not installation-auth/ref-pinned content.
- Behind main; rebase.
bdd649c to
35b4a82
Compare
…SONbored#830) Add opt-in reviewer-routing feature with three modes: off (default), advisory (surface CODEOWNERS suggestions in panel only), and auto_request (also call GitHub request-reviewers API for the top-ranked suggestion). - migrations/0046: add reviewer_routing_mode column (default 'off') - src/types.ts: ReviewerRoutingMode union + optional field on RepositorySettings - src/db/schema.ts + repositories.ts: parseReviewerRoutingMode, full read/write wiring - src/signals/reviewer-routing.ts: CODEOWNERS parser + buildReviewerRouting ranker - src/github/reviewer-request.ts: fetchCodeownersFile, getRequestedReviewers, requestPullRequestReviewers - src/signals/focus-manifest.ts: expose reviewerRoutingMode in settings: block - src/signals/settings-preview.ts + src/openapi/schemas.ts: include in preview/API surface - src/queue/processors.ts: auto_request fires after label step; newcomer guard (0 merged PRs → skip), already-requested reviewers skipped (idempotent), teams skipped at CODEOWNERS parse time, best-effort (failures audited, never abort) - test/unit/reviewer-routing.test.ts: parseCodeowners + buildReviewerRouting unit tests covering newcomer-guard invariants, idempotency, glob semantics, ranking Closes JSONbored#830
35b4a82 to
2122855
Compare




Summary
Implements the opt-in reviewer-routing feature end-to-end, closing #830 (follow-up to the deferred
auto_requestaction from #540).Adds a new
reviewerRoutingModesetting with three modes:off(default)advisoryauto_requestThe
auto_requestmode includes three hard safety invariants from the issue spec:authorHistorydetection already threaded for feat(github-app): first-time-contributor-aware gating #552@org/team) are excluded at CODEOWNERS parse time; only individual user logins reach the APIRelated Issue
Change Type
.gittensory.ymlsettings:block)Real Behaviour Proof
CODEOWNERS parsing (unit-testable, no network)
Newcomer guard (verified by test)
Idempotency guard (verified by test)
Migration — additive only
Checklist
ALTER TABLE … ADD COLUMN … DEFAULT 'off') — zero downtime, safe rollbackreviewerRoutingModedefaults to"off"everywhere (DB default, code default,getRepositorySettingsmissing-row path) — existing repos are unaffectedauto_requestblock is best-effort: wrapped intry/catch, failures are audited viarecordAuditEventand never abort the main surface publishauthorHistory.mergedPrCountalready computed for feat(github-app): first-time-contributor-aware gating #552 — no extra DB querygetRequestedReviewersis called before everyrequestPullRequestReviewers@org/team) excluded at CODEOWNERS parse time, never reach the GitHub APIreviewersarrayreviewerRoutingModeis surfaced in.gittensory.ymlsettings:block viaparseSettingsOverrideRepositorySettingsandRepoSettingsPreviewSchemaRepositorySettings(matchesbadgeEnabledprecedent) — zero existing test fixture changes needed