Skip to content

feat(root): adds better management support for security#452

Merged
yowainwright merged 5 commits intomainfrom
new-security-features
Mar 24, 2026
Merged

feat(root): adds better management support for security#452
yowainwright merged 5 commits intomainfrom
new-security-features

Conversation

@yowainwright
Copy link
Copy Markdown
Owner

Proposed Changes

  • Adds more nuaced support for security and override updates

Read about referenced issues here. Replace words with this Pull Request's context.

Comment thread tests/integration/e2e-cli.test.ts Fixed
Comment thread src/core/update/index.ts Outdated
Comment thread src/core/appendix/utils.ts
Comment thread src/config/constants.ts Outdated
@yowainwright yowainwright force-pushed the new-security-features branch from 17049ad to ede9330 Compare March 23, 2026 18:17
Repository owner deleted a comment from sentry bot Mar 23, 2026
Repository owner deleted a comment from greptile-apps bot Mar 23, 2026
@sentry
Copy link
Copy Markdown

sentry bot commented Mar 23, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.14%. Comparing base (a1939dd) to head (749eafc).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #452      +/-   ##
==========================================
+ Coverage   97.04%   97.14%   +0.10%     
==========================================
  Files          61       61              
  Lines        8312     8618     +306     
==========================================
+ Hits         8066     8372     +306     
  Misses        246      246              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@yowainwright
Copy link
Copy Markdown
Owner Author

@greptile

@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Mar 24, 2026

Greptile Summary

This PR is a well-scoped security management upgrade that migrates the entire codebase from a single cve string field to a cves array, adds per-CVE cveDetails objects, and introduces keep / KeepConstraint support so users can pin overrides that must survive --remove-unused. A new checkRemovalSafety helper runs a synthetic security scan on candidate-unused entries before they are removed, and stepUpdateKeptOverrides keeps potentiallyFixedIn current across runs. All three issues flagged in the previous review (wrong keep !== true guard, isKeepExpired never called, duplicate KeepConstraint definition) have been resolved.

Key changes:

  • cvecves migration: All four providers (OSV, GitHub, Snyk, Socket), type definitions, ledger, and display layer updated to the array form; normalizeLedgerCveField handles existing data without a migration step.
  • KeepConstraint support: isKeptEntry and isKeepExpired are now wired into isUnusedEntry, so both keep: true and time/version-bounded constraints are respected by --remove-unused.
  • checkRemovalSafety: Before removal, unused entries whose packages are still in direct dependencies are re-checked via the active security provider; any still-vulnerable entries are added to skipRemovalKeys.
  • stepUpdateKeptOverrides: New pipeline step that cross-references live security alerts against kept entries' CVEs to keep potentiallyFixedIn fresh.
  • Two issues remain: deduplicateAlerts silently drops CVEs from lower-severity duplicate alerts (see inline comment), and buildCveDetails can produce duplicate entries when multiple detail objects share a CVE.

Confidence Score: 4/5

  • Safe to merge with one targeted fix — the CVE merge gap in deduplicateAlerts can cause silent data loss in a narrow but real scenario.
  • All three prior review concerns are resolved, the new keep/KeepConstraint pipeline is well-tested with e2e round-trip tests, and the provider migrations are straightforward. The one logic issue (deduplicateAlerts drops CVEs from the losing branch) could cause CVE identifiers to be silently omitted from the ledger when two vulnerability records for the same package share a first CVE alias but carry different additional ones — a real but narrow scenario. The cveDetails deduplication gap is cosmetic. Neither breaks the primary security-override path.
  • src/core/security/utils.ts — the deduplicateAlerts CVE merge branch.

Important Files Changed

Filename Overview
src/core/security/utils.ts CVE merge in deduplicateAlerts only fires when the incoming alert wins (higher severity); CVEs from lower-severity duplicate alerts are silently dropped.
src/core/appendix/utils.ts Major improvements: adds isKeptEntry, isKeepExpired (now wired into isUnusedEntry), normalizeLedgerCveField, and multi-detail CVE aggregation. Minor: buildCveDetails does not deduplicate entries when multiple details share the same CVE.
src/core/update/index.ts Adds stepUpdateKeptOverrides (updates potentiallyFixedIn using live alerts) and passes skipRemovalKeys into stepRemoveUnused. Previous issues with KeepConstraint guard and isKeptEntry usage are fully resolved.
src/cli/index.ts Adds checkRemovalSafety helper that runs a synthetic security scan on candidate-unused entries before removal, and wires result into skipRemovalKeys. CVE fields migrated to array form throughout.
src/types.ts Introduces KeepConstraint, CveDetail, and expanded AppendixItem.ledger fields (securityCheckResult, cves, cveDetails, keep, potentiallyFixedIn, resolution fields). Clean, well-structured additions.
src/config/constants.ts Re-exports KeepConstraint from src/types.ts (resolving the previous duplicate definition) and expands AppendixItem.ledger validation with clean isFieldValid helpers covering all new fields.
src/core/security/providers/osv.ts Migrates from extractCVE (returns first alias) to extractCVEs (returns all CVE aliases). Correctly omits the field when the array is empty.
src/core/security/providers/github.ts Wraps the advisory's single cve_id in an array and only adds the field when non-empty. Correct migration to the cves array shape.
src/core/security/providers/snyk.ts Migrates from the first CVE identifier to the full identifiers.CVE array. Correctly omits the field when empty.
src/core/security/providers/socket.ts Wraps the socket issue's single CVE in a one-element array when present, omits it otherwise. Clean migration.
src/dx/terminal-graph.ts Updates display helpers to use cves array, adds formatKeepStatus and formatPotentiallyFixedIn for richer override display, and adjusts icon selection to use info for kept entries.
tests/integration/e2e-cli.test.ts Adds comprehensive e2e tests covering keep round-trips, removeUnused skip behaviour, cveDetails/vulnerableRange population, OSV scan pipeline, and legacy cvecves normalisation.

Sequence Diagram

sequenceDiagram
    participant CLI as cli/index.ts
    participant SC as SecurityChecker
    participant CRS as checkRemovalSafety
    participant UPD as update pipeline
    participant SKO as stepUpdateKeptOverrides
    participant SRU as stepRemoveUnused

    CLI->>SC: checkSecurity(config)
    SC-->>CLI: alerts (cves[], patchedVersion, vulnerableRange)
    CLI->>CLI: buildSecurityResult(alerts)
    CLI->>CLI: mergedOptions ← securityAlerts

    opt removeUnused && config
        CLI->>CRS: checkRemovalSafety(config, securityChecker, options)
        CRS->>CRS: findUnusedAppendixEntries(appendix)
        CRS->>SC: checkSecurity(syntheticConfig)
        SC-->>CRS: alerts for candidate packages
        CRS-->>CLI: skipKeys (still-vulnerable unused entries)
        CLI->>CLI: mergedOptions ← skipRemovalKeys
    end

    CLI->>UPD: update(mergedOptions)
    UPD->>SKO: stepUpdateKeptOverrides(ctx)
    SKO->>SKO: match kept entries' cves against live alerts
    SKO-->>UPD: updated potentiallyFixedIn in appendix

    UPD->>SRU: stepRemoveUnused(ctx)
    SRU->>SRU: findUnusedAppendixEntries(appendix, rootDeps)
    SRU->>SRU: filter out skipRemovalKeys
    SRU->>SRU: warn on CVE-tracked removals
    SRU-->>UPD: finalAppendix / finalOverrides
Loading

Fix All in Claude Code

Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/core/security/utils.ts
Line: 36-45

Comment:
**CVE merge only fires on the winning branch**

When `shouldReplace` is `false` (the existing alert has equal or higher severity), the incoming alert's CVEs are silently dropped. Two alerts for the same package can legitimately share a first CVE but carry different additional CVEs — for example, OSV can return separate vulnerability records that alias to `["CVE-A", "CVE-B"]` and `["CVE-A", "CVE-C"]` respectively. Processing the higher-severity alert first then encountering the lower-severity one would lose `CVE-C` entirely.

The CVE merge needs to happen unconditionally:

```suggestion
    if (shouldReplace) {
      const mergedCves = existing
        ? [...new Set([...(existing.cves || []), ...(alert.cves || [])])]
        : alert.cves;
      const merged =
        mergedCves && mergedCves.length > 0
          ? { ...alert, cves: mergedCves }
          : alert;
      map.set(key, merged);
    } else if (existing && alert.cves?.length) {
      const mergedCves = [...new Set([...(existing.cves || []), ...alert.cves])];
      map.set(key, { ...existing, cves: mergedCves });
    }
```

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: src/core/appendix/utils.ts
Line: 147-156

Comment:
**`cveDetails` can contain duplicate entries**

`buildCveDetails` flat-maps every CVE across all `SecurityOverrideDetail` objects without deduplication. If two detail objects for the same package both carry the same CVE (e.g. two providers or two vulnerability records sharing an alias), the resulting `cveDetails` array will contain duplicate entries — while the sibling `cves` flat list is correctly deduplicated via `new Set`.

Consider deduplicating by CVE identifier:

```ts
const buildCveDetails = (details: SecurityOverrideDetail[]): CveDetail[] => {
  const map = new Map<string, CveDetail>();
  for (const d of details) {
    for (const cve of d.cves || []) {
      if (!map.has(cve)) {
        const detail: CveDetail = { cve };
        if (d.severity) detail.severity = d.severity;
        if (d.patchedVersion) detail.patchedVersion = d.patchedVersion;
        map.set(cve, detail);
      }
    }
  }
  return Array.from(map.values());
};
```

How can I resolve this? If you propose a fix, please make it concise.

Reviews (2): Last reviewed commit: "feat(root): adds more code optimizations" | Re-trigger Greptile

Comment thread src/core/security/utils.ts
@yowainwright yowainwright merged commit 77169e6 into main Mar 24, 2026
18 checks passed
@yowainwright yowainwright deleted the new-security-features branch March 24, 2026 07:38
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.

2 participants