Skip to content

[codex] Fix manifest CLI release defaults#260

Merged
Navi Bot (project-navi-bot) merged 2 commits into
mainfrom
codex/release-audit-followup
Jun 19, 2026
Merged

[codex] Fix manifest CLI release defaults#260
Navi Bot (project-navi-bot) merged 2 commits into
mainfrom
codex/release-audit-followup

Conversation

@Fieldnote-Echo

Copy link
Copy Markdown
Member

Summary

This follows up on the v0.5.0 release audit findings that were still actionable after the repository transfer and recent release-pipeline fixes.

Changed:

  • make ordvec-manifest default to the cli feature, so cargo install ordvec-manifest installs the advertised binary
  • keep the library-only path available with default-features = false
  • document the new default install path and remove stale --features cli examples for normal CLI use
  • build ordvec-manifest docs.rs with the intended public feature surface: cli plus sqlite-bundled
  • declare the standalone fuzz crate MSRV as Rust 1.89
  • extend release invariants so the CLI default, docs.rs feature surface, and fuzz MSRV stay synchronized

Audit disposition

Fixed in this PR:

  • finding 1: ordvec-manifest CLI was not installed by default Cargo behavior
  • finding 6: manifest docs.rs omitted intended CLI/SQLite public surfaces
  • finding 7: fuzz package did not declare the project MSRV

Investigated but not changed here:

  • finding 2: current release.yml already publishes core crate, manifest crate, core PyPI wheels, and manifest PyPI wheels; C/Go/native-library distribution remains a release-scope/product decision rather than a single-cargo-publish bug
  • finding 3: bare workspace cargo commands are still not the release gate; CI/release invariants use explicit lanes for Rust, FFI, Python, Go, fuzz, CodeQL, actionlint, and zizmor
  • finding 4: coverage upload is still intentionally non-blocking; this remains a release-process decision if we want a blocking release-only coverage proof
  • finding 5: manifest publication already waits on core publication and retries registry/package visibility instead of relying on a fixed sleep only

Validation

  • cargo fmt --all --check
  • git diff --check HEAD
  • python3 tests/release_publish_invariants.py
  • bash tests/release_publish_invariants.sh
  • cargo install --path ordvec-manifest --root /tmp/ordvec-manifest-install-test --locked
  • /tmp/ordvec-manifest-install-test/bin/ordvec-manifest --version
  • cargo test -p ordvec-manifest --no-default-features --locked
  • cargo test -p ordvec-manifest --all-features --locked
  • cargo clippy -p ordvec-manifest --all-targets --all-features --locked -- -D warnings
  • cargo doc -p ordvec-manifest --features cli,sqlite-bundled --no-deps --locked

Note: a broader local cargo test --workspace --all-features --locked was also probed during investigation and failed in this environment on ordvec-python PyO3 linker symbols. That failure is not caused by this patch and is why the release matrix continues to rely on the dedicated Python/maturin lanes for Python coverage.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

Fix ordvec-manifest CLI default features and release invariants
🐞 Bug fix 📝 Documentation ⚙️ Configuration changes 🧪 Tests 🕐 40+ Minutes

Grey Divider

Description

• Make ordvec-manifest install the advertised CLI by default via cli default feature.
• Align docs.rs feature surface with intended public API (cli + sqlite-bundled).
• Extend release invariants to keep manifest defaults, docs.rs features, and fuzz MSRV synchronized.
Diagram

graph TD
A["Release invariants"] --> B["ordvec-manifest Cargo.toml"] --> C["Crate defaults / docs.rs"]
A --> D["ordvec-manifest README"]
A --> E["Repo docs (docs/*.md)"]
A --> F["fuzz/Cargo.toml"] --> G["MSRV Rust 1.89"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Split into separate CLI crate (e.g., ordvec-manifest-cli)
  • ➕ Keeps the library crate’s default feature set minimal without requiring default-features = false for most library users
  • ➕ Avoids pulling clap into the default feature surface entirely
  • ➖ Adds another published crate to version, document, and keep in lockstep
  • ➖ Can complicate discoverability and release automation (two packages instead of one)
2. Keep default features empty and require `--features cli` everywhere
  • ➕ Maintains strict library-first default behavior
  • ➕ Avoids changing the default feature surface for existing dependents
  • cargo install ordvec-manifest would still not install the advertised binary by default
  • ➖ More friction for the primary user path (CLI install/run), and more likely to regress again

Recommendation: The chosen approach (defaulting ordvec-manifest to cli while preserving default-features = false for library-only consumers) is the best trade-off: it matches Cargo’s default install expectations for the published binary, keeps an escape hatch for dependency-minimizing consumers, and the added release invariants make regressions unlikely.

Files changed (6) +61 / -17

Tests (1) +41 / -0
release_publish_invariants.pyAdd invariants for fuzz MSRV, docs.rs features, and manifest CLI defaults +41/-0

Add invariants for fuzz MSRV, docs.rs features, and manifest CLI defaults

• Extends compatibility checks to assert 'fuzz/Cargo.toml' declares the repo MSRV. Tightens registry/docs.rs metadata validation to require 'ordvec-manifest' docs.rs features be exactly '["cli", "sqlite-bundled"]' and forbids 'features' keys elsewhere. Adds a dedicated check ensuring 'ordvec-manifest' defaults to 'cli', that 'cli' enables 'dep:clap', that the binary remains gated behind 'required-features = ["cli"]', and that the README doesn’t require '--features cli' for installs.

tests/release_publish_invariants.py

Documentation (3) +17 / -16
INDEX_PROVENANCE.mdRemove '--features cli' from manifest CLI usage example +1/-1

Remove '--features cli' from manifest CLI usage example

• Updates the documented verification command to reflect that the manifest crate now enables the CLI by default. Keeps the example aligned with typical workspace usage ('cargo run -p ordvec-manifest -- ...').

docs/INDEX_PROVENANCE.md

msrv-and-features.mdDocument manifest default CLI feature and include fuzz crate in MSRV scope +9/-8

Document manifest default CLI feature and include fuzz crate in MSRV scope

• Updates the MSRV policy text to include the standalone 'ordvec-fuzz' crate. Adjusts the feature-surface table and narrative to state that 'ordvec-manifest' defaults to 'cli' and that library-only users should set 'default-features = false'.

docs/msrv-and-features.md

README.mdUpdate install/run instructions for new default CLI behavior +7/-7

Update install/run instructions for new default CLI behavior

• Rewrites README guidance to use 'cargo install ordvec-manifest' and removes now-stale '--features cli' examples. Adds explicit guidance for library-only consumers to use 'default-features = false'.

ordvec-manifest/README.md

Other (2) +3 / -1
Cargo.tomlDeclare fuzz crate MSRV as Rust 1.89 +1/-0

Declare fuzz crate MSRV as Rust 1.89

• Adds 'rust-version = "1.89"' to the fuzz package to explicitly align it with the repository MSRV policy and invariant checks.

fuzz/Cargo.toml

Cargo.tomlEnable CLI by default and pin docs.rs feature surface +2/-1

Enable CLI by default and pin docs.rs feature surface

• Sets 'features.default = ["cli"]' so 'cargo install ordvec-manifest' installs the binary without extra flags. Configures docs.rs builds to use 'features = ["cli", "sqlite-bundled"]' with 'all-features = false' so published docs reflect the intended public surface.

ordvec-manifest/Cargo.toml

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. Brittle Cargo.toml checks 🐞 Bug ☼ Reliability
Description
check_manifest_cli_defaults() validates ordvec-manifest’s binary feature-gating by searching for raw
substrings in Cargo.toml, so harmless formatting/quoting changes can fail release invariants. It
also doesn’t reliably verify the [[bin]] entry because name = "ordvec-manifest" can match the
[package] name instead of the bin target.
Code

tests/release_publish_invariants.py[R566-569]

+    text = read_text("ordvec-manifest/Cargo.toml")
+    if 'name = "ordvec-manifest"' not in text or 'required-features = ["cli"]' not in text:
+        fail("ordvec-manifest/Cargo.toml: binary must remain gated on the cli feature")
+
Relevance

⭐⭐⭐ High

Team accepted hardening brittle invariant string-matching (PR173) and improving TOML parsing
robustness (PR180).

PR-#173
PR-#180

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The invariant currently depends on exact string presence in the TOML text, which is sensitive to
non-semantic edits and can match the package name rather than the [[bin]] table. The manifest
already has a structured [[bin]] stanza that tomllib can parse, so the check should inspect that
structure instead of substrings.

tests/release_publish_invariants.py[552-574]
ordvec-manifest/Cargo.toml[1-27]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`check_manifest_cli_defaults()` currently uses raw substring matches against `ordvec-manifest/Cargo.toml` to ensure the CLI binary remains gated on the `cli` feature. This is brittle (formatting/whitespace/quoting changes break it) and imprecise (`name = "ordvec-manifest"` can match the package name, not the `[[bin]]` entry).

## Issue Context
The rest of `release_publish_invariants.py` already parses TOML via `load_toml()`, so we can reliably inspect the parsed `bin` array.

## Fix Focus Areas
- tests/release_publish_invariants.py[552-569]
- ordvec-manifest/Cargo.toml[23-27]

## Suggested change
- Replace the raw `read_text(...)/"..." in text` check with TOML-structure validation:
 - Read `bins = sequence(manifest.get("bin"), "ordvec-manifest/Cargo.toml: bin")` (or allow missing -> fail with a clear message).
 - Find the bin entry where `name == "ordvec-manifest"`.
 - Validate `required-features` is a string sequence containing `"cli"` (or exactly `["cli"]` if you want strictness).
 - Fail with a targeted error if the bin target is missing or not gated.
- (Optional) If you still want to guard against removal of the bin name, assert on the parsed value rather than a textual substring.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

@project-navi-bot Navi Bot (project-navi-bot) added the review-this Trigger OpenHands PR review label Jun 19, 2026

@github-actions github-actions 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.

Clean, well-scoped PR that addresses the three release audit findings:

  1. ordvec-manifest CLI defaultdefault = ["cli"] plus required-features = ["cli"] on the binary ensures cargo install ordvec-manifest works out of the box while library consumers can opt out with default-features = false.

  2. docs.rs feature surfacefeatures = ["cli", "sqlite-bundled"] exposes the intended public API on docs.rs, consistent with the feature matrix.

  3. fuzz MSRVrust-version = "1.89" added to fuzz/Cargo.toml and synchronized via the existing check_release_compatibility_sync() invariant.

The new check_manifest_cli_defaults() test is a solid guard against drift. All validation commands in the PR description pass. No issues found.


Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/Project-Navi/ordvec/actions/runs/27849729950

@project-navi-bot Navi Bot (project-navi-bot) merged commit a640779 into main Jun 19, 2026
37 checks passed
@project-navi-bot Navi Bot (project-navi-bot) deleted the codex/release-audit-followup branch June 19, 2026 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

review-this Trigger OpenHands PR review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants