Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions .gitattributes
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
* text=auto eol=lf
*.rs text diff=rust
*.toml text
*.md text
*.yml text eol=lf
*.yaml text eol=lf
*.json text
*.sh text eol=lf
*.bat text eol=crlf
Cargo.lock binary linguist-generated=false

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggestion: Cargo.lock is marked as binary, but lockfiles are plain text and are expected to be diffed and merged in PRs. Treating it as binary disables normal text diff/merge behavior, which can hide dependency changes and cause avoidable merge conflicts; classify it as text instead. [logic error]

Severity Level: Major ⚠️
- ⚠️ Dependency update PRs hide Cargo.lock textual changes.
- ⚠️ Git cannot auto-merge Cargo.lock on conflicts.
- ⚠️ Reviewers may miss important dependency version changes.
Steps of Reproduction ✅
1. In the repo root
`/tmp/pr-review/repo-clones/KooshaPari/helios-cli/1e2f9e755ed378f41dda0f4cbdf873bc411225ca`,
note there is a Rust manifest `Cargo.toml` (listed by `LS` at the root, alongside
`.gitattributes`).

2. Generate and commit a lockfile by running `cargo generate-lockfile` (which creates
`Cargo.lock` next to `Cargo.toml`), then run `git add Cargo.lock`.

3. Because `.gitattributes` line 10 (`.gitattributes:10 Cargo.lock binary
linguist-generated=false`) marks `Cargo.lock` as `binary`, run `git diff --cached` and
observe that Git shows a binary diff summary (e.g., "Binary files differ") instead of a
normal line-based text diff for `Cargo.lock`.

4. Create a branch that also modifies dependencies (regenerating `Cargo.lock`), then merge
the branches; Git again treats `Cargo.lock` as binary due to `.gitattributes:10`,
preventing normal line-level merge and forcing manual conflict resolution without readable
textual context of dependency changes.

Fix in Cursor | Fix in VSCode Claude

(Use Cmd/Ctrl + Click for best experience)

Prompt for AI Agent 🤖
This is a comment left during a code review.

**Path:** .gitattributes
**Line:** 10:10
**Comment:**
	*Logic Error: `Cargo.lock` is marked as `binary`, but lockfiles are plain text and are expected to be diffed and merged in PRs. Treating it as binary disables normal text diff/merge behavior, which can hide dependency changes and cause avoidable merge conflicts; classify it as text instead.

Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
👍 | 👎

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Contradictory Cargo.lock attributes hide PR diffs

Low Severity

The Cargo.lock binary linguist-generated=false rule combines two attributes that work against each other. The binary macro disables text diff/merge in git, while linguist-generated=false instructs GitHub to treat the file as non-generated and therefore not collapse it in PR views. The net effect is that Cargo.lock changes are surfaced in PRs as "Binary file changed" with no line-level content, which obstructs review of dependency updates and makes it harder to catch malicious or unintended cargo audit-relevant changes.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 1e2f9e7. Configure here.

package-lock.json binary linguist-generated=true
*.png binary
*.jpg binary
10 changes: 10 additions & 0 deletions .github/workflows/cargo-semver-checks.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
name: cargo-semver-checks
on:
pull_request: { paths: ['**/Cargo.toml'] }
workflow_dispatch:
jobs:
semver-checks:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- uses: obi1kenobi/cargo-semver-checks-action@v2

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Semver checks workflow targets stub root workspace

High Severity

The new cargo-semver-checks workflow runs without specifying working-directory or a manifest-path input, so cargo-semver-checks-action resolves the workspace at the repo root. That root workspace contains only the dummy helios stub package; the real Rust crates live under codex-rs/. The workflow therefore never validates semver compatibility for any of the actual codex-rs crates, even though the trigger fires on **/Cargo.toml changes (including those under codex-rs).

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 1e2f9e7. Configure here.

9 changes: 9 additions & 0 deletions Taskfile.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
version: '3'
tasks:
default: { desc: List tasks, cmds: [task --list-all] }
build: { desc: cargo build, cmds: [cargo build --workspace] }
test: { desc: cargo test, cmds: [cargo test --workspace] }
lint: { desc: cargo clippy + fmt --check, cmds: [cargo clippy --workspace -- -D warnings, cargo fmt --check] }
fmt: { desc: cargo fmt, cmds: [cargo fmt] }
audit: { desc: cargo deny + audit, cmds: [cargo deny check, cargo audit] }
ci: { desc: lint + test + audit, cmds: [{task: lint}, {task: test}, {task: audit}] }

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Taskfile targets stub workspace instead of codex-rs

High Severity

The Taskfile.yml and cargo-semver-checks workflow run cargo commands from the repository root, targeting a stub Cargo.toml instead of the active workspace in codex-rs/. This means local build, test, lint, and audit tasks provide false confidence or fail, and semver checks in CI miss actual crate changes.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 1e2f9e7. Configure here.

Loading