Skip to content

Reduce dependency metadata false positives#793

Merged
wesm merged 5 commits into
mainfrom
teal-tarn
Jun 5, 2026
Merged

Reduce dependency metadata false positives#793
wesm merged 5 commits into
mainfrom
teal-tarn

Conversation

@wesm
Copy link
Copy Markdown
Member

@wesm wesm commented Jun 5, 2026

Summary

  • Preserve dependency consistency evidence in review prompts without inlining large lockfile/checksum bodies.
  • Add a compact Dependency Metadata section that lists changed manifests, lockfiles, and checksums, including explicit notes when package.json or go.mod changes lack companion metadata changes.
  • Restore lockfile/checksum body exclusions in review diffs while covering the new behavior with prompt and git tests.

Context

This targets CI review false positives where roborev hid package-lock.json or go.sum changes, causing reviewers to infer dependency bumps were incomplete.

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Jun 5, 2026

roborev: Combined Review (a458ce1)

Medium-risk prompt coverage issues need fixing before merge; no security findings were reported.

Medium

  • internal/prompt/prompt.go:509
    Dirty reviews still exclude lockfile/checksum bodies via GetDirtyDiff, but BuildDirty has no dependency metadata summary path. As a result, uncommitted lockfile-only changes can disappear from the prompt entirely.
    Fix: Capture dirty changed file names before applying review excludes and pass them into dirty prompt construction so DependencyMetadata is populated. Add a dirty prompt test covering excluded lockfiles.

  • internal/prompt/dependency_metadata.go:98
    package.json companion detection only checks the same directory, which can incorrectly report “no JavaScript lockfile change detected” for common workspace layouts where packages/foo/package.json changes with a root pnpm-lock.yaml, yarn.lock, or package-lock.json.
    Fix: Treat JavaScript lockfiles in ancestor/root workspace locations as companions, or suppress the missing-lockfile warning when any recognized JavaScript lockfile changed in the same range.


Panel: ci_default_security | Synthesis: codex, 9s | Members: codex_default (codex/default, done, 2m22s), codex_security (codex/security, done, 1m10s) | Total: 3m41s

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Jun 5, 2026

roborev: Combined Review (39e43b7)

Summary verdict: The PR has medium-risk correctness issues around dirty diff handling and reruns; no security issues were found.

Medium

  • cmd/roborev/review.go:301, internal/daemon/panel_enqueue.go:201
    Empty dirty diffs are now accepted whenever any dirty file exists, but dirtyFiles is unfiltered and can contain only non-dependency excluded paths like .cache/ or .beads/. That can produce a prompt with no diff and no dependency metadata.

    Suggested fix: Only allow diffContent == "" when the dirty file list contains dependency metadata that will actually be summarized; otherwise keep the previous “diff is empty” error.

  • internal/daemon/panel_enqueue.go:520
    Dirty daemon prompts are prebuilt with BuildDirtyWithFiles, bypassing the existing BuildDirtyWithSnapshot path. Large dirty diffs under the enqueue size limit but over the prompt cap can lose the full external diff snapshot, leaving the agent with only truncated content.

    Suggested fix: Let the worker build dirty prompts with BuildDirtyWithSnapshotAndFiles, or add durable dirty-diff snapshot handling for prebuilt dirty prompts.

  • internal/daemon/server.go:1836
    Metadata-only dirty jobs rely on a prebuilt prompt while DirtyFiles is not persisted. Rerunning the job clears the prebuilt prompt, leaving a dirty job with no DiffContent and no file list to rebuild the metadata prompt.

    Suggested fix: Persist the dirty file list or preserve/rebuild the prebuilt dirty prompt for reruns of metadata-only dirty jobs.


Panel: ci_default_security | Synthesis: codex, 11s | Members: codex_default (codex/default, done, 4m28s), codex_security (codex/security, done, 2m8s) | Total: 6m47s

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Jun 5, 2026

roborev: Combined Review (49dadc0)

Summary verdict: One medium correctness issue needs attention; no security findings were reported.

Medium

  • internal/git/git.go:778: git status --porcelain collapses untracked directories, so GetDirtyFilesChanged may record frontend/ instead of files like frontend/package-lock.json. Dirty reviews can then miss or reject metadata-only changes inside newly untracked directories.
    • Suggested fix: Use git status --porcelain=v1 -uall, or combine tracked name-only output with git ls-files --others --exclude-standard so untracked files are expanded.

Panel: ci_default_security | Synthesis: codex, 6s | Members: codex_default (codex/default, done, 5m44s), codex_security (codex/security, done, 2m12s) | Total: 8m2s

@roborev-ci
Copy link
Copy Markdown

roborev-ci Bot commented Jun 5, 2026

roborev: Combined Review (d62d1c1)

No Medium, High, or Critical findings were reported.

All review outputs are clean at the requested severity threshold.


Panel: ci_default_security | Synthesis: codex, 5s | Members: codex_default (codex/default, done, 8m44s), codex_security (codex/security, done, 2m7s) | Total: 10m56s

@wesm wesm merged commit 084102e into main Jun 5, 2026
4 checks passed
@wesm wesm deleted the teal-tarn branch June 5, 2026 03:08
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.

1 participant