Skip to content

feat(enrichment): cross-file caller-impact / dead-symbol analyzer (#1509) - #1780

Closed
dev-miro26 wants to merge 11 commits into
JSONbored:mainfrom
dev-miro26:feat/enrichment-caller-impact
Closed

feat(enrichment): cross-file caller-impact / dead-symbol analyzer (#1509)#1780
dev-miro26 wants to merge 11 commits into
JSONbored:mainfrom
dev-miro26:feat/enrichment-caller-impact

Conversation

@dev-miro26

Copy link
Copy Markdown
Contributor

Summary

Adds the cross-file caller-impact / dead-symbol analyzer to the review-enrichment service (REES), resolving #1509. It surfaces two cross-file hazards the no-checkout in-prompt reviewer (which only sees the diff) is blind to:

  • Removed / renamed / signature-changed export with live callers in unchanged files — a hidden compile/runtime break. Callers are resolved on the repo's default branch via the GitHub Code Search API (where the pre-existing, about-to-break callers live), then filtered to files the PR did not touch. Reported as removed-with-callers or changed-with-callers with the unchanged caller file paths.
  • Dead-on-arrival — a newly-exported symbol referenced nowhere in the PR. Code Search indexes the default branch only, so a brand-new symbol is invisible to it; this case is therefore judged from the diff (the new export is dead if no added line outside its own declaration references it), and public-entrypoint files (index.*, *.d.ts) are skipped because their exports are intentionally unused internally.

It reports symbol names + unchanged caller file paths only — never source. Self-contained and fail-safe, matching the codeowners (#1515) / asset-weight (#1621) / history-analyzer precedent: it uses the request's short-lived githubToken for Code Search and degrades gracefully — no token → [], and a failed / rate-limited search drops only that symbol while the rest of the brief still ships. No engine / src/** change; entirely within review-enrichment/ (its own node:test suite, outside the main tsc/vitest/Codecov scope).

Wired per the issue: CallerImpactFinding type + callerImpact key in types.ts, scanCallerImpact registered in the brief.ts analyzer registry, and a public-safe block in render.ts. 18 node:test units cover export-declaration parsing (every form, aliases, re-export-all, anonymous default), removed/changed/moved classification, the dead-on-arrival diff scan (incl. the public-entrypoint skip and $-identifier boundary), the no-token / rate-limit degrade paths, the SSRF repo guard, and rendering.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format.
  • 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. (Closes feat(enrichment): Cross-file caller-impact / dead-symbol analyzer #1509)

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck
  • npm run test:coverage — full suite green. No src/** changed, so there is no codecov/patch obligation (the analyzer lives in review-enrichment/, which Codecov ignores).
  • 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 — 0 vulnerabilities
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries — review-enrichment suite green (200 pass, 18 new), covering every branch / fallback / degrade path.

If any required check was skipped, explain why:

  • The whole npm run test:ci chain is green on this branch (off current main); npm run rees:test (build + sourcemap validation + node:test) also run directly.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed. The brief carries only symbol names + public file paths.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics. The rendered block uses the existing safeCodeSpan escaper and is re-sanitized + defanged by the engine before it is spliced.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests. — N/A: no auth/cookie/CORS/session change.
  • API/OpenAPI/MCP behavior is updated and tested where needed. The REES contract (BriefFindings.callerImpact) is extended and unit-tested; no main-repo OpenAPI/MCP surface is touched.
  • UI changes use live API data or real empty/error/loading states. — N/A: no UI change.
  • Visible UI changes include a UI Evidence section. — N/A: backend-only (review-enrichment service); no visible UI / frontend / docs / extension change.
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs. No changelog edited.

UI Evidence

N/A — backend-only change to the review-enrichment service; no visible UI, frontend, docs, or extension surface.

Notes

  • Code Search vs diff split (by design): removed/renamed/changed exports use Code Search on the default branch, which is exactly where the pre-existing callers live; dead-on-arrival is judged from the diff because Code Search cannot see a symbol that does not yet exist on the default branch. Both are bounded (≤ 8 symbols searched, to respect the Code Search rate budget) and fail-safe.
  • The analyzer is additive + inert until enabled per-repo via the engine's REES_ANALYZERS setting, exactly like the existing analyzers — keeping this a clean, zero-engine-conflict drop entirely within review-enrichment/.
  • Branch is off current main; no base conflict.

@dev-miro26
dev-miro26 requested a review from JSONbored as a code owner June 29, 2026 22:33
@dosubot dosubot Bot added the size:L label Jun 29, 2026
@loopover-orb

loopover-orb Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Caution

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

🛑 Gittensory review result - reject/close recommended

Review updated: 2026-06-30 06:13:11 UTC

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

🛑 Suggested Action - Reject/Close

  • AI reviewers agree on a likely critical defect: review-enrichment/src/analyzers/caller-impact.ts:456 skips every export whose file/name key still exists, so an exported function/type whose signature changes keeps the same key and never produces the advertised changed-with-callers finding for unchanged callers. — Resolve the flagged defect, or override if the AI reviewers are mistaken, then re-run the gate.

Review summary
This adds and wires a new caller-impact analyzer plus rendering and tests, and the overall fail-safe shape is consistent with the existing enrichment analyzers. The core removed-export path now verifies candidate files by fetching content and resolving relative import specifiers, which avoids several obvious same-name false positives. However, the implementation still misses the advertised signature-change class entirely and misinterprets aliased named imports, so the analyzer can both fail open on a required hazard and emit false caller findings.

Blockers

  • review-enrichment/src/analyzers/caller-impact.ts:456 skips every export whose file/name key still exists, so an exported function/type whose signature changes keeps the same key and never produces the advertised changed-with-callers finding for unchanged callers.
  • review-enrichment/src/analyzers/caller-impact.ts:170 treats any occurrence of the symbol inside an import clause as a dependency on that exported symbol, so `import { other as removedName } from './lib'` is falsely reported as depending on a removed `removedName` export.
Nits — 6 non-blocking
  • nit: review-enrichment/src/analyzers/caller-impact.ts:359 tracks dead-on-arrival by symbol name only, so if the same new export name appears in multiple files the entrypoint skip and file attribution depend on whichever file is seen first.
  • nit: review-enrichment/src/analyzers/caller-impact.ts:140 only strips single-line template literals, so a multiline template containing a new export name can suppress a dead-on-arrival finding even though it is not code usage.
  • nit: review-enrichment/src/render.ts:369 renders all caller files into one line; the analyzer caps this today, but keeping a local render cap would protect the brief if the finding contract grows later.
  • In review-enrichment/src/analyzers/caller-impact.ts, add real changed-with-callers support or remove it from the PR contract/type narrative; detecting declaration text changes before continuing at line 456 is the minimum fix.
  • In review-enrichment/src/analyzers/caller-impact.ts, parse named import/export specifiers into imported and local names so only the imported/exported side is compared for removed symbols.
  • 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/analyzers/caller-impact.ts:456 skips every export whose file/name key still exists, so an exported function/type whose signature changes keeps the same key and never produces the advertised changed-with-callers finding for unchanged callers.
Signal Result Evidence
Code review ❌ 1 blocker 1 reviewer
Linked issue ✅ Linked #1509
Related work ⚠️ 3 scoped overlaps Top overlaps are listed below; lower-confidence bulk is hidden.
Change scope ❌ 8/20 High review scope from cached public metadata (size label size:XL; 1 linked issue).
Validation posture ❌ 5/25 Preflight is holding this PR; address the blocker before review.
Contributor workload ✅ 10/10 Author activity: 6 registered-repo PR(s), 4 merged, 1 issue(s).
Contributor context ✅ Confirmed Gittensor contributor dev-miro26; Gittensor profile; 6 PR(s), 1 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Review context
Contributor next steps
  • Review top overlaps.
  • Add a concise scope and risk note.
  • Fix the blocker.
  • Triage stale or unlinked PRs.
  • 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.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide 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

@loopover-orb loopover-orb Bot added gittensor gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 0.25x multiplier. labels Jun 29, 2026
@dosubot dosubot Bot added the size:L label Jun 29, 2026
@dev-miro26

Copy link
Copy Markdown
Contributor Author

I will fix this conflicts soon

…ler-impact

# Conflicts:
#	review-enrichment/src/brief.ts
#	review-enrichment/src/render.ts
#	review-enrichment/src/types.ts
@dosubot dosubot Bot added size:XL and removed size:L labels Jun 30, 2026
@dosubot dosubot Bot added size:M and removed size:XL labels Jun 30, 2026
@dosubot dosubot Bot removed the size:M label Jun 30, 2026
@dosubot dosubot Bot added the size:XL label Jun 30, 2026

@JSONbored JSONbored left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Merge conflicts, and blockers in review comment:

This branch has conflicts that must be resolved
Use the [web editor](https://github.com/JSONbored/gittensory/pull/1780/conflicts) or the command line to resolve conflicts before continuing.

review-enrichment/src/brief.ts

@JSONbored JSONbored closed this Jun 30, 2026
@github-project-automation github-project-automation Bot moved this from Todo to Done in gittensory - v1 roadmap Jun 30, 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 — scores a 0.25x multiplier.

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

feat(enrichment): Cross-file caller-impact / dead-symbol analyzer

2 participants