Skip to content

ci: catch conflict markers and check desktop rust#8030

Open
Git-on-my-level wants to merge 4 commits into
mainfrom
ci/conflict-rust-guards
Open

ci: catch conflict markers and check desktop rust#8030
Git-on-my-level wants to merge 4 commits into
mainfrom
ci/conflict-rust-guards

Conversation

@Git-on-my-level

@Git-on-my-level Git-on-my-level commented Jun 19, 2026

Copy link
Copy Markdown
Collaborator

Summary

Adds static guards for the failure mode caught on #7951:

  • fail CI/pre-commit when changed/staged files contain unresolved merge conflict markers (<<<<<<< / >>>>>>>)
  • run rustfmt on changed macOS desktop Rust backend files
  • run cargo check --locked for macOS desktop Rust backend changes
  • dogfood the Rust check when .github/workflows/lint.yml itself changes, so this PR measures the new CI cost
  • cache Cargo registry/git/target directories in CI to reduce repeat-run cost

Why

The existing lint workflow only covered Dart, backend Python, ARB, firmware C/C++, and selected web packages. Rust files under desktop/macos/Backend-Rust/**/*.rs could pass PR lint even with syntax errors or conflict markers.

Local validation / timing

sh -n scripts/pre-commit
python3 - <<'PY'
import yaml
with open('.github/workflows/lint.yml') as f:
    yaml.safe_load(f)
print('lint.yml parses as YAML')
PY

Result: passed.

Conflict-marker scan over this PR's changed files:

/usr/bin/time -f 'elapsed=%E user=%U sys=%S' <scan command>

Result: elapsed=0:00.00 user=0.00 sys=0.00

Pre-commit hook on this PR's staged files (no Rust files staged, so conflict scan only):

/usr/bin/time -f 'elapsed=%E user=%U sys=%S' scripts/pre-commit

Result: elapsed=0:00.01 user=0.00 sys=0.01

I also verified the new conflict-marker pattern catches the current #7951 markers in desktop/macos/Backend-Rust/src/routes/chat_completions.rs.

CI timing from this PR

This PR intentionally ran the desktop Rust backend guard because .github/workflows/lint.yml changed.

  • First attempt using cargo fmt --check across the whole crate failed in 43s due to existing unrelated rustfmt drift.
  • Final implementation limits formatting to changed Rust files and keeps full cargo check --locked.
  • Final Lint & Format Check: passed in 1m33s.
  • Check desktop Rust backend step: 48s.

The universal conflict-marker CI step completed within the same second as changed-file detection in GitHub's step timestamps.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

3 issues found across 2 files

Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic

Comment thread scripts/pre-commit Outdated
Comment thread scripts/pre-commit Outdated
Comment thread .github/workflows/lint.yml Outdated
@Git-on-my-level

Copy link
Copy Markdown
Collaborator Author

Update after first CI run:

  • The initial cargo fmt --check across the whole desktop Rust crate failed in 43s because the existing crate has unrelated rustfmt drift.
  • I changed the formatting guard to rustfmt --check only the changed Rust files, while keeping cargo check --locked for the crate parse/type check.
  • The workflow still dogfoods cargo check on this PR because .github/workflows/lint.yml changed.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 33724761d6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread .github/workflows/lint.yml Outdated
if: steps.changed.outputs.has_desktop_rust == 'true'
working-directory: desktop/macos/Backend-Rust
run: |
cargo fmt --check

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Format the Rust crate before enabling fmt

Because this PR also sets has_desktop_rust for .github/workflows/lint.yml changes, this new step runs on the PR itself. cargo fmt --help says it formats all bin/lib files of the current crate, and I ran cargo fmt --check in desktop/macos/Backend-Rust; it exits nonzero with rustfmt diffs across existing files such as src/auth.rs and src/services/firestore.rs. As written, the lint job is blocked before it reaches cargo check until the existing crate is formatted or the check is scoped differently.

Useful? React with 👍 / 👎.

Comment thread .github/workflows/lint.yml Outdated
echo "has_personas=$(echo "$FILES" | grep -q '^web/personas-open-source/' && echo true || echo false)" >> $GITHUB_OUTPUT
# Also run the Rust check when this workflow changes so CI PRs dogfood
# the new guard instead of only validating YAML syntax.
echo "has_desktop_rust=$(echo "$FILES" | grep -qE '^desktop/macos/Backend-Rust/.*\.rs$|^\.github/workflows/lint\.yml$' && echo true || echo false)" >> $GITHUB_OUTPUT

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Include Rust manifests in the backend check

This trigger only matches changed .rs files or this workflow file, so changes to desktop/macos/Backend-Rust/Cargo.toml or Cargo.lock skip the new cargo check --locked step. A dependency/version/feature edit can therefore introduce lockfile drift or a build failure while the Rust CI path stays disabled; include the Rust manifests (and any build script if added) in this predicate.

Useful? React with 👍 / 👎.

@Git-on-my-level

Copy link
Copy Markdown
Collaborator Author

Addressed the new review comments:

  • fixed staged conflict-marker scan to iterate quoted paths, including filenames with spaces
  • fixed desktop Rust pre-commit check so cargo check --locked failure exits the hook
  • widened desktop Rust CI/pre-commit trigger from only *.rs to any desktop/macos/Backend-Rust/ change, covering Cargo.toml / Cargo.lock
  • Codex's earlier whole-crate fmt concern was already handled by scoping rustfmt to changed Rust files

Validation:

sh -n scripts/pre-commit
lint.yml parses as YAML
staged filename with spaces + conflict markers: hook failed as expected
precommit elapsed=0:00.01 user=0.01 sys=0.01

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

1 issue found across 2 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

Fix all with cubic | Re-trigger cubic

Comment thread scripts/pre-commit Outdated

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: be6245106f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread .github/workflows/lint.yml Outdated
run: |
FILES=$(grep -E '^desktop/macos/Backend-Rust/.*\.rs$' /tmp/changed-files.txt || true)
if [ -n "$FILES" ]; then
echo "$FILES" | xargs rustfmt --check

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Pass the Rust edition to rustfmt

For changed desktop Rust files this invokes rustfmt directly, so it does not read the crate's edition = "2021" from Cargo.toml; rustfmt --help exposes --edition, and locally rustfmt --check desktop/macos/Backend-Rust/src/routes/health.rs fails with async fn is not permitted in Rust 2015 even though the crate compiles as Rust 2021. Any PR touching an async .rs file will therefore fail this lint step before cargo check; pass --edition 2021 here or run formatting through Cargo with the crate manifest.

Useful? React with 👍 / 👎.

@Git-on-my-level

Copy link
Copy Markdown
Collaborator Author

Addressed the latest review comments:

  • rustfmt in pre-commit is now enforced per file; if formatting fails, the hook prints the file path and exits non-zero.
  • Direct rustfmt invocations now pass --edition 2021 in both pre-commit and CI, so async Rust files are parsed with the crate's edition instead of rustfmt's default.

Validation:

sh -n scripts/pre-commit
lint.yml parses as YAML
precommit elapsed=0:00.01 user=0.00 sys=0.01

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

1 issue found across 2 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name=".github/workflows/lint.yml">

<violation number="1" location=".github/workflows/lint.yml:142">
P2: The CI rustfmt invocation is not path-safe: `xargs` splits filenames on whitespace, so changed Rust files with spaces in their path will break formatting checks.</violation>
</file>

Tip: Review your code locally with the cubic CLI to iterate faster.

Fix all with cubic | Re-trigger cubic

run: |
FILES=$(grep -E '^desktop/macos/Backend-Rust/.*\.rs$' /tmp/changed-files.txt || true)
if [ -n "$FILES" ]; then
echo "$FILES" | xargs rustfmt --edition 2021 --check

@cubic-dev-ai cubic-dev-ai Bot Jun 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: The CI rustfmt invocation is not path-safe: xargs splits filenames on whitespace, so changed Rust files with spaces in their path will break formatting checks.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/lint.yml, line 142:

<comment>The CI rustfmt invocation is not path-safe: `xargs` splits filenames on whitespace, so changed Rust files with spaces in their path will break formatting checks.</comment>

<file context>
@@ -139,7 +139,7 @@ jobs:
           FILES=$(grep -E '^desktop/macos/Backend-Rust/.*\.rs$' /tmp/changed-files.txt || true)
           if [ -n "$FILES" ]; then
-            echo "$FILES" | xargs rustfmt --check
+            echo "$FILES" | xargs rustfmt --edition 2021 --check
           fi
           cd desktop/macos/Backend-Rust
</file context>
Suggested change
echo "$FILES" | xargs rustfmt --edition 2021 --check
printf '%s\n' "$FILES" | while IFS= read -r file; do rustfmt --edition 2021 --check "$file"; done
Fix with cubic

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 254c2bc8fe

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread scripts/pre-commit
conflict_found=0
while IFS= read -r file; do
[ -n "$file" ] || continue
if git grep --cached -n -I -E '^(<<<<<<<|>>>>>>>)' -- "$file"; then

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Re-scan conflict markers after staging formatter output

This checks only the cached index before the hook's later formatter blocks mutate the index with git add, so a partially staged firmware file can still commit markers: stage a clean hunk in omi/*.c, leave an unstaged <<<<<<< hunk, and the later clang-format -i/git add path stages the whole file after this scan has already passed (I checked clang-format exits 0 and leaves that marker line intact). Move this scan to the end or rerun it after every formatter has re-staged files.

Useful? React with 👍 / 👎.

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