Skip to content

feat(enrichment): revert-recurrence detector for re-introduced revert…#1687

Open
dale053 wants to merge 8 commits into
JSONbored:mainfrom
dale053:feat/enrichment-revert-recurrence-detector
Open

feat(enrichment): revert-recurrence detector for re-introduced revert…#1687
dale053 wants to merge 8 commits into
JSONbored:mainfrom
dale053:feat/enrichment-revert-recurrence-detector

Conversation

@dale053

@dale053 dale053 commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Adds a revertRecurrence analyzer to the review-enrichment service (REES) that detects when a PR re-introduces code that was previously reverted — a known-problematic path being re-trodden without addressing the original reason it was walked back.
  • The analyzer fetches per-file commit history from the GitHub commits API, identifies revert commits (message starts with "Revert"), fetches their diffs, and intersects the lines they removed with the lines being added by the current PR. A match is reported as a RevertRecurrenceFinding.
  • Findings appear in the promptSection under "### Re-introduced reverted code (known-problematic path re-trodden)" and in the structured findings.revertRecurrence array.
  • Bounded: MAX_FILES=10, MAX_COMMITS_PER_FILE=30, MAX_REVERT_CHECKS_PER_FILE=5, MAX_FINDINGS=15. Requires at least MIN_MATCH_LINES=2 non-trivial lines (MIN_LINE_LEN=8) to suppress coincidental hits on common structural patterns.
  • Fail-safe throughout: non-ok API responses, network throws, and non-array commit-list responses all degrade silently to empty findings — no false positives from infrastructure noise.

Closes #1514

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
  • npm run typecheck
  • npm run test:coverage locally; codecov/patch requires ≥97% coverage of the lines AND branches you changed (aim for 98%+ 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

If any required check was skipped, explain why:

  • npm run typecheck and npm run ui:typecheck report 28 and 5 pre-existing errors respectively (missing optional self-host packages: pg, ioredis, @sentry/node, @cloudflare/puppeteer, @testing-library/react). These errors are identical on main before this PR and are unrelated to the changes here. Zero new type errors introduced.
  • All changes are confined to review-enrichment/ which is outside the main vitest coverage scope (src/**) and the Codecov include path. The enrichment test suite (node --test in review-enrichment/) runs 70 tests, all passing, with every new branch in revert-recurrence.ts covered.

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. — N/A: no auth/CORS/session changes.
  • API/OpenAPI/MCP behavior is updated and tested where needed. — N/A: no API schema changes; the enrichment service is an internal analysis layer, not a public endpoint change.
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks. — N/A: no UI changes.
  • Visible UI changes include a UI Evidence section below with JPG/JPEG or PNG screenshots. — N/A: no visible UI changes.
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs.

UI Evidence

N/A — no visible UI, frontend, docs, or extension changes.

Notes

  • The analyzer uses the githubToken from EnrichRequest when present (adds Authorization: Bearer … header); omits the header for public repos where no token is provided.
  • Revert detection is content-based (normalized line intersection), not line-number-range-based. This is intentional: content matching is robust across rebases and merge commits, where line numbers shift but the reverted code remains recognisable.
  • Only the file path, revert commit SHA (7 chars), and match count are stored in RevertRecurrenceFinding — no code content is ever captured or rendered, preserving the same value-redaction principle used by the secret analyzer.
  • The Array.isArray guard in listFileCommits ensures that a mocked or unexpected non-array response from the GitHub API does not propagate as an uncaught TypeError into buildBrief, keeping the analyzer isolated from upstream format surprises.

…ed code (JSONbored#1514)

Adds a REES analyzer that fetches per-file commit history from the GitHub
commits API, identifies revert commits (message starts with "Revert"),
fetches their diffs, and intersects the lines they removed with the lines
being added by the current PR. A hit indicates known-problematic code is
being re-introduced without addressing the original reason it was reverted.

- New `RevertRecurrenceFinding` interface and `revertRecurrence` key in `BriefFindings`
- Pure helper exports (`isRevertMessage`, `extractAddedLines`, `extractRemovedLines`)
  enable direct unit testing without network mocks
- Fail-safe: non-ok responses, network throws, and non-array commit-list responses
  all degrade silently to empty findings
- Bounded: MAX_FILES=10, MAX_COMMITS_PER_FILE=30, MAX_REVERT_CHECKS_PER_FILE=5,
  MAX_FINDINGS=15; MIN_MATCH_LINES=2 + MIN_LINE_LEN=8 suppress coincidental hits
- Auth header included when `githubToken` is present; omitted for public repos
- 70 tests all passing; all new branches covered
@dale053 dale053 requested a review from JSONbored as a code owner June 28, 2026 14:56
@dosubot dosubot Bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jun 28, 2026

@superagent-security superagent-security Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Superagent found 1 security concern(s).

Comment thread review-enrichment/src/analyzers/revert-recurrence.ts Outdated
@superagent-security superagent-security Bot added the pr:flagged PR flagged for review by security analysis. label Jun 28, 2026
@gittensory-orb

gittensory-orb Bot commented Jun 28, 2026

Copy link
Copy Markdown

Caution

🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥🟥

🛑 Gittensory review result - reject/close recommended

5 files · 1 AI reviewer · 1 blocker · readiness 55/100 · CI green · blocked

🛑 Suggested Action - Reject/Close — AI reviewers agree on a likely critical defect: review-enrichment/src/types.ts:111 leaves `RevertRecurrenceFinding` unclosed, so `ProvenanceFinding` is parsed inside the previous interface and the file is invalid TypeScript. — Resolve the flagged defect, or override if the AI reviewers are mistaken, then re-run the gate.

Review updated: 2026-06-29 11:16:22 UTC

Review summary
The change adds the new revert-recurrence analyzer and wires it into brief generation, but the post-change TypeScript is structurally broken. The analyzer logic itself is coherent, but the rendered findings block and the new type definition are missing closing braces, so the changed files cannot parse as written.

Blockers

  • review-enrichment/src/types.ts:111 leaves `RevertRecurrenceFinding` unclosed, so `ProvenanceFinding` is parsed inside the previous interface and the file is invalid TypeScript.
  • review-enrichment/src/render.ts:169 does not close the `for` loop and enclosing `if` for `revertRecurrences`, so the rest of `renderBrief` is nested incorrectly and the function is syntactically invalid.
Nits — 4 non-blocking
  • Add the missing `}` after `matchedLines: number;` in `review-enrichment/src/types.ts:111`.
  • Close the `lines.push(...)`, `for`, and `if` block before `const provenance = findings.provenance ?? [];` in `review-enrichment/src/render.ts:169`.
  • AI maintainer-assist flagged possible low-effort patterns (elevated) — Advisory only — review the noted patterns; this AI assist never blocks the gate.
  • Readiness score is below the configured threshold — Use the readiness panel as advisory maintainer context; the score does not block this PR.

Why this is blocked

  • review-enrichment/src/types.ts:111 leaves `RevertRecurrenceFinding` unclosed, so `ProvenanceFinding` is parsed inside the previous interface and the file is invalid TypeScript.
Signal Result Evidence
Code review ❌ 1 blocker 1 reviewer
Linked issue ✅ Linked #1514
Related work ⚠️ 3 scoped overlaps Top overlaps are listed below; lower-confidence bulk is hidden.
Review load ❌ 8/20 Readiness component derived from cached public PR metadata and labels; size label size:L.
Validation evidence ❌ 5/25 Cached preflight status is hold.
Open PR queue ❌ 3/10 19 open PR(s), 7 likely reviewable, 12 unlinked.
Contributor context ✅ Confirmed Gittensor contributor dale053; Gittensor profile; 44 PR(s), 20 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Review context
Contributor next steps
  • Review top overlaps.
  • Add scope summary.
  • Fix blocker.
  • Expect slower review.
  • Refresh registry data or choose a registered active repo.
  • Check active issues and PRs before submitting.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Review load = cached public PR metadata such as size labels, changed paths, and preflight status.
  • Open PR queue = repo-wide review pressure; it is not a PR quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 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.

  • Re-run Gittensory review

@gittensory-orb gittensory-orb Bot added gittensor Gittensor contributor context gittensor:feature Gittensor-scored feature linked to a feature issue - worth 1.25x multiplier. labels Jun 28, 2026
@superagent-security superagent-security Bot removed the pr:flagged PR flagged for review by security analysis. label Jun 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:feature Gittensor-scored feature linked to a feature issue - worth 1.25x multiplier. gittensor Gittensor contributor context size:L This PR changes 100-499 lines, ignoring generated files.

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

feat(enrichment): Revert-recurrence detector

2 participants