Skip to content

fix(rees): scan added lines whose content starts with ++ across all diff parsers#2531

Merged
JSONbored merged 1 commit into
JSONbored:mainfrom
glorydavid03023:fix/rees-plus-guard-all-parsers
Jul 2, 2026
Merged

fix(rees): scan added lines whose content starts with ++ across all diff parsers#2531
JSONbored merged 1 commit into
JSONbored:mainfrom
glorydavid03023:fix/rees-plus-guard-all-parsers

Conversation

@glorydavid03023

@glorydavid03023 glorydavid03023 commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Summary

Every review-enrichment diff parser guarded file headers with an unanchored startsWith("+++"). git renders an added line whose content is ++x as + + ++x = +++x (and ++ x as +++ x), so that added line was mistaken for a +++ b/file header and skipped. In secret-scan.ts a secret on such a line was never scanned (scanSecrets calls scanPatch directly).

Fix, by parser input shape:

  • Hunk-structured parsers (secret-scan, secret-log, iac-misconfig, redos, actions-pin, eol-check, duplication-scan, shared analysis-context) track hunk state: headers only precede the first @@, and inside a hunk the first char is the +/-/space op, so both +++x and +++ x are scanned.
  • Parsers that also accept header-only / headerless fragments (history, heavy-dependency) use a shared isDiffFileHeaderLine helper matching only a real header form (+++ b/… / --- a/… / /dev/null), so +++ x content is kept while true headers are skipped — and headerless single-line diffs still work.
  • Manifest/lockfile parsers (dependency-scan, lockfile-drift) keep an anchored +++ guard; their content never begins a line with ++.
scanPatch("src/config.ts", '@@ -1,0 +1,1 @@
++ const key = "AKIA…";')  // before: []  after: [aws_access_key_id]

This is the complete follow-up to the earlier narrower attempt (which fixed only analysis-context and left secret-scan's own scanPatch). Resolves the reviewer blockers on secret-scan, history, and heavy-dependency (each skipped the reachable ++ x content shape).

No issue because issue creation is restricted on this repo for outside accounts; this consolidates a duplicated diff-parsing guard.

Scope

  • Conventional Commit title; focused; follows CONTRIBUTING; small self-evident fix (no linked issue).

Validation

  • git diff --check
  • npm run typecheck (rees tsc -p tsconfig.json clean)
  • npm run test:coverage (N/A — review-enrichment/** is outside Codecov's src/**/*.ts include)
  • npm audit --audit-level=moderate
  • New/changed behavior has tests

If any required check was skipped, explain why:

  • Change confined to the rees package. node --test passes 391/393; the only 2 failures are test/sentry-upload.test.ts (Sentry CLI binary absent locally), identical on unmodified main. Added regressions: secret-scan.test.ts (secret on ++x- and ++ x-content lines), analysis-context.test.ts (collectAddedLines/filesHaveAddedLines), and a diff-lines.test.ts unit for the header discriminator. review-enrichment/** is outside Codecov's src/** include. UI/workers/MCP steps are unrelated to this rees-only diff.

Safety

  • No secrets/wallets/hotkeys/trust/reward/private terms exposed (test fixtures build fake keys from fragments at runtime).
  • Public GitHub text stays sanitized and low-noise.
  • Auth/cookie/CORS/session negative-path tests. — N/A.
  • API/OpenAPI/MCP behavior updated/tested where needed. (None changed.)
  • UI changes use live API data. — N/A.
  • UI Evidence. — N/A (no visible UI change).
  • Docs/changelog updated where needed. (None needed.)

Notes

  • The shared isDiffFileHeaderLine helper (new analyzers/diff-lines.ts) prevents the guard from drifting again across parsers.

@dosubot dosubot Bot added the size:S label Jul 2, 2026
@loopover-orb

loopover-orb Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-02 09:08:16 UTC

16 files · 1 AI reviewer · no blockers · readiness 75/100 · CI green · clean

⏸️ Suggested Action - Manual Review

Review summary
The change correctly moves most hunk-structured analyzers away from prefix-based `+++` header skipping and adds regression coverage for the common `++x` / `++ x` added-line shapes. The shared header helper is a reasonable fit for parsers that intentionally accept headerless fragments, but its path-form heuristic still has a narrow false-header case for real added content that begins with the same `++ b/` or `++ /dev/null` text. Overall the visible fix covers the reported reachable secret-scan path, with one follow-up worth tightening in the shared helper/tests.

Nits — 4 non-blocking
  • nit: `review-enrichment/src/analyzers/diff-lines.ts:15` still classifies added content like `++ b/foo` rendered as `+++ b/foo` as a file header, so the helper should document that tradeoff or tighten matching to real parser context where possible.
  • nit: `review-enrichment/test/diff-lines.test.ts:14` should include edge cases for content rendered as `+++ b/not-a-header` and `+++ /dev/null is text` so the remaining heuristic boundary is explicit.
  • In `review-enrichment/src/analyzers/diff-lines.ts:15`, consider requiring callers with full unified patches to use hunk state and reserve `isDiffFileHeaderLine` only for genuinely headerless inputs, or add a stricter discriminator if the source format provides filename context.
  • In `review-enrichment/test/diff-lines.test.ts:14`, add regression cases for `+++ b/...`-looking added content and decide whether those are acceptable misses or should be scanned.
Signal Result Evidence
Code review ✅ No blockers 1 reviewer
Linked issue ✅ No-issue rationale PR body explains why no issue is linked.
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (size label size:M; no linked issue context).
Validation posture ❌ 5/25 Preflight is holding this PR: the review lane is unavailable, so it is not ready for automated review.
Contributor workload ✅ 10/10 Author activity: 213 registered-repo PR(s), 142 merged, 9 issue(s).
Contributor context ✅ Confirmed Gittensor contributor glorydavid03023; Gittensor profile; 213 PR(s), 9 issue(s).
Gate result ✅ Passing No configured blocker found.
Review context
  • Author: glorydavid03023
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: JavaScript, Python, TypeScript, C++, Kotlin, MDX, Rust
  • Official Gittensor activity: 213 PR(s), 9 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Await review-lane availability.
  • Triage stale or unlinked PRs.
  • Refresh registry data or choose a registered active repo.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
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:bug Gittensor-scored bug fix — scores a 0.05x multiplier. labels Jul 2, 2026
@glorydavid03023
glorydavid03023 force-pushed the fix/rees-plus-guard-all-parsers branch from 7bbecb9 to 81e54b2 Compare July 2, 2026 08:59
@dosubot dosubot Bot added size:M and removed size:S labels Jul 2, 2026
…iff parsers

Every review-enrichment diff parser guarded file headers with an unanchored
`startsWith("+++")`. git renders an added line whose content is `++x` as
`+` + `++x` = `+++x` (and `++ x` as `+++ x`), so that line was mistaken for a
`+++ b/file` header and skipped. In secret-scan.ts a secret on such a line was
never scanned (scanSecrets calls scanPatch directly).

Fixes, by parser input shape:
- Hunk-structured parsers (secret-scan, secret-log, iac-misconfig, redos,
  actions-pin, eol-check, duplication-scan, shared analysis-context) track hunk
  state: headers only precede the first @@, and inside a hunk the first char is
  the +/-/space op, so both `+++x` and `+++ x` are scanned.
- Parsers that also accept header-only/headerless fragments (history,
  heavy-dependency) use a shared isDiffFileHeaderLine helper that matches only a
  real header form (`+++ b/…`/`--- a/…`/`/dev/null`), so `+++ x` content is kept
  while true headers are skipped — and headerless single-line diffs still work.
- Manifest/lockfile parsers (dependency-scan, lockfile-drift) keep an anchored
  `+++ ` guard; their content never begins a line with `++`.

Adds regression tests: scanPatch finds a secret on both `++x`- and `++ x`-content
lines, collectAddedLines recovers the `++x` line, and isDiffFileHeaderLine has a
unit covering headers vs `++`/`--` content and headerless diffs.

No issue because issue creation is restricted on this repo; this consolidates a
duplicated diff-parsing guard, no schema or API change.
@dosubot dosubot Bot added the lgtm label Jul 2, 2026
@JSONbored
JSONbored merged commit ccd23d5 into JSONbored:main Jul 2, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Development

Successfully merging this pull request may close these issues.

2 participants