From ec2ec480d82aa82cd478e6e4e8034fabe4706372 Mon Sep 17 00:00:00 2001 From: politerealism Date: Wed, 19 Aug 2026 12:43:36 -0400 Subject: [PATCH 1/2] docs: clarify vouch timing, DCO vs signing, Cargo feature conventions - Strengthen vouch timing guidance: explicitly note that auto-closed PRs lose their discussion thread and must be manually reopened - Clarify DCO vs. cryptographic signing: both are separate requirements with separate failure modes; --signoff is universal, GPG signing is org-member-only for copy-pr-bot mirroring - Add Cargo feature conventions section covering feature vs. build mode distinction, transitive dependency auditing, and CI verification - Add PR testing checklist guidance: only mark items complete after actual verification, not as a plan No issue required: documentation-only clarifications to contributor workflow derived from observed friction in recent PRs. Signed-off-by: politerealism --- CONTRIBUTING.md | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4befd7b4a..d92487e5d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -26,9 +26,9 @@ We use a vouch system. This exists because AI makes it trivial to generate plaus 2. Describe what you want to change and why. 3. Write in your own words. AI-generated vouch requests will be denied. 4. A maintainer will comment `/vouch` if approved. -5. Once vouched, you can submit pull requests. +5. **Wait for approval before opening a PR.** Once vouched, you can submit pull requests. -**If you are not vouched, any pull request you open will be automatically closed.** Org members and collaborators with push access bypass this check. +**If you are not vouched, any pull request you open will be automatically closed.** Closing loses the discussion thread and the PR must be manually reopened — it cannot be converted to a draft or held. Org members and collaborators with push access bypass this check. ### Finding Work @@ -509,10 +509,22 @@ chore(deps): bump tokio to 1.40 All human contributions must include a `Signed-off-by` line in each commit message. This certifies you have the right to submit the work under the project license. See the [Developer Certificate of Origin](https://developercertificate.org/). Dependabot-authored dependency update PRs are allowlisted because the bot cannot sign commits. ```bash -git commit -s -m "feat(sandbox): add new capability" +git commit --signoff -m "feat(sandbox): add new capability" ``` -DCO sign-off is separate from cryptographic commit signing. CI requires signing for org members so that copy-pr-bot can mirror your PR automatically; see [CI.md](CI.md#commit-signing) for setup. +DCO sign-off (`--signoff`) and cryptographic commit signing are separate requirements with separate failure modes. The `--signoff` flag adds the `Signed-off-by` trailer checked by the DCO Assistant bot — a missing or mismatched trailer will fail the DCO check regardless of whether the commit is GPG-signed. Cryptographic signing is required for org members only, so that copy-pr-bot can mirror your PR to run CI; see [CI.md](CI.md#commit-signing) for setup. + +### Cargo feature conventions + +When adding or changing Cargo features that affect packaging or distribution: + +- **Feature vs. build mode:** A feature opt-in adds a capability. Removing a feature to disable something (e.g., bundled CA roots) is a build mode — document it as such rather than creating a mirror feature just to name the absence. A convenience alias that re-includes all other defaults is appropriate when distro recipes would otherwise need to manually track every unrelated default feature. +- **Transitive dependency auditing:** When a feature is meant to exclude a dependency, verify it is physically absent — not just disabled at the call site. Run `cargo tree -e features -p ` to confirm the excluded crate does not appear through a transitive path. A common source of leaks: crates that enable `rustls-tls` or similar features by default in their own `[features]` table. +- **CI verification:** Add a `mise` task or CI step that builds without the feature and confirms the excluded dependency is absent from `cargo tree` output. A compile-time guard alone may not catch a transitive re-introduction. + +### PR testing checklist + +Only mark a checklist item as complete after you have actually run and verified it — not as a plan for what you intend to test. Unchecked boxes at submission are fine; they signal what still needs verification before merge. ## CI From d6710048267f6e607876dee5e6c93de2c6053245 Mon Sep 17 00:00:00 2001 From: politerealism Date: Thu, 20 Aug 2026 14:03:43 -0400 Subject: [PATCH 2/2] docs: address review feedback on contributing guide improvements - Expand -m to --message in the DCO commit example for consistency with --signoff. - Rephrase vouch step 5 from "Wait for approval" to "Get vouched" to clarify that contributors can prepare code locally while waiting. - Move Cargo feature conventions out of CONTRIBUTING.md (too language-specific for a project-wide guide) into architecture/build.md under the existing Build Features section, rewritten in prose style to match that document's conventions. Signed-off-by: politerealism --- CONTRIBUTING.md | 12 ++---------- architecture/build.md | 11 +++++++++++ 2 files changed, 13 insertions(+), 10 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d92487e5d..818da2d9a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -26,7 +26,7 @@ We use a vouch system. This exists because AI makes it trivial to generate plaus 2. Describe what you want to change and why. 3. Write in your own words. AI-generated vouch requests will be denied. 4. A maintainer will comment `/vouch` if approved. -5. **Wait for approval before opening a PR.** Once vouched, you can submit pull requests. +5. **Get vouched before opening a PR.** Once vouched, you can submit pull requests. **If you are not vouched, any pull request you open will be automatically closed.** Closing loses the discussion thread and the PR must be manually reopened — it cannot be converted to a draft or held. Org members and collaborators with push access bypass this check. @@ -509,19 +509,11 @@ chore(deps): bump tokio to 1.40 All human contributions must include a `Signed-off-by` line in each commit message. This certifies you have the right to submit the work under the project license. See the [Developer Certificate of Origin](https://developercertificate.org/). Dependabot-authored dependency update PRs are allowlisted because the bot cannot sign commits. ```bash -git commit --signoff -m "feat(sandbox): add new capability" +git commit --signoff --message "feat(sandbox): add new capability" ``` DCO sign-off (`--signoff`) and cryptographic commit signing are separate requirements with separate failure modes. The `--signoff` flag adds the `Signed-off-by` trailer checked by the DCO Assistant bot — a missing or mismatched trailer will fail the DCO check regardless of whether the commit is GPG-signed. Cryptographic signing is required for org members only, so that copy-pr-bot can mirror your PR to run CI; see [CI.md](CI.md#commit-signing) for setup. -### Cargo feature conventions - -When adding or changing Cargo features that affect packaging or distribution: - -- **Feature vs. build mode:** A feature opt-in adds a capability. Removing a feature to disable something (e.g., bundled CA roots) is a build mode — document it as such rather than creating a mirror feature just to name the absence. A convenience alias that re-includes all other defaults is appropriate when distro recipes would otherwise need to manually track every unrelated default feature. -- **Transitive dependency auditing:** When a feature is meant to exclude a dependency, verify it is physically absent — not just disabled at the call site. Run `cargo tree -e features -p ` to confirm the excluded crate does not appear through a transitive path. A common source of leaks: crates that enable `rustls-tls` or similar features by default in their own `[features]` table. -- **CI verification:** Add a `mise` task or CI step that builds without the feature and confirms the excluded dependency is absent from `cargo tree` output. A compile-time guard alone may not catch a transitive re-introduction. - ### PR testing checklist Only mark a checklist item as complete after you have actually run and verified it — not as a plan for what you intend to test. Unchecked boxes at submission are fine; they signal what still needs verification before merge. diff --git a/architecture/build.md b/architecture/build.md index 5c5751772..1a1e1ce7f 100644 --- a/architecture/build.md +++ b/architecture/build.md @@ -58,6 +58,17 @@ HTTP/TLS support behind explicit build features, so default system-Z3 builds do not reintroduce bundled Mozilla roots. Release builds that need bundled Z3 continue to opt in with `bundled-z3`. +A feature opt-in adds a capability; removing a feature to disable something is a +build mode, not the absence of a feature flag. Document it as such rather than +creating a mirror feature just to name the absence. When a feature is meant to +exclude a dependency, verify the crate is physically absent from the dependency +graph — not just disabled at the call site. Use `cargo tree -e features -p +` to confirm the excluded crate does not appear through a transitive path. +A common source of leaks is crates that enable `rustls-tls` or similar features +by default in their own `[features]` table. Verify the exclusion holds in CI with +a `mise` task that builds without the feature and checks `cargo tree` output; a +compile-time guard alone does not catch a transitive re-introduction. + ## Linux Runtime Environments OpenShell uses different Linux libc environments for different host artifacts.