Centralize branding metadata from workspace manifest#1392
Merged
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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 on lines
26
to
27
| workspace_root.display() | ||
| ); |
There was a problem hiding this comment.
When the build script cannot determine the workspace root via git (for example, when compiling from a source tarball or in environments that lack a .git directory or the git binary), it falls back to manifest_dir and still calls load_workspace_metadata on that directory. The crate‐local Cargo.toml does not contain [workspace.metadata.oc_rsync], so the new code panics with “workspace.metadata.oc_rsync missing …” and the build aborts. Previous versions compiled fine from a vendored tree; now any build without git present will fail. Consider searching upward for the real workspace root or providing defaults instead of hard failing in this scenario.
Useful? React with 👍 / 👎.
4 tasks
oferchen
added a commit
that referenced
this pull request
May 1, 2026
Adds an integration test suite covering the cross-layer ACL negotiation flow when the remote peer was compiled without `--enable-acl-support`. Upstream rsync gates ACL transfer on two layers: 1. Protocol-version gate (compat.c:655-661) - errors when proto < 30. 2. Local-binary feature gate (options.c:1842-1857 + acls.c:25,1141) - when the peer was built without SUPPORT_ACLS, the entire acls.c compilation unit is empty and the binary never advertises `-A` in the compact server flag string. oc-rsync mirrors layer (1) verbatim in `transfer::setup::restrictions::apply_protocol_restrictions` and covers layer (2) in `ParsedServerFlags::clear_unsupported_features` plus the warning emission in `transfer::lib`. The new tests (`crates/transfer/tests/acl_negotiation_missing_remote.rs`) exercise the cross-layer behaviour end-to-end so future refactors cannot silently regress to either: - panicking when the peer advertises no ACL support, or - sending ACL bytes on the wire when the peer cannot decode them. Coverage: - Wire-level: server flag string without `A` yields acls=false. - Restriction layer: protocol 30+ does NOT reject --acls. - Restriction layer: protocol < 30 rejects with the exact upstream error message format byte-for-byte. - Restriction layer: local_server bypasses the protocol gate at any protocol (matching upstream's `!local_server` guard). - Cross-layer: protocol 32 + remote without `-A` results in graceful skip, no panic, no spurious warnings, no wire-level ACL data. - Ordering: when both gates fire (proto 29 + remote without -A), the protocol-version error wins, matching upstream's check order. - Idempotency: clear_unsupported_features must not double-report when the flag was already false from the wire. Findings: - oc-rsync matches upstream behaviour for the rejection path (byte-for-byte error message at restrictions.rs:96-101). - For the local-feature path, oc-rsync chooses graceful fallback (warn + continue) where upstream errors out - this is documented in flags.rs:144-146 and is the user-preferred behaviour. - Gate locations: crates/transfer/src/setup/restrictions.rs:96-101 (protocol gate) and crates/transfer/src/flags.rs:153 (local feature gate).
6 tasks
oferchen
added a commit
that referenced
this pull request
May 5, 2026
Adds an integration test suite covering the cross-layer ACL negotiation flow when the remote peer was compiled without `--enable-acl-support`. Upstream rsync gates ACL transfer on two layers: 1. Protocol-version gate (compat.c:655-661) - errors when proto < 30. 2. Local-binary feature gate (options.c:1842-1857 + acls.c:25,1141) - when the peer was built without SUPPORT_ACLS, the entire acls.c compilation unit is empty and the binary never advertises `-A` in the compact server flag string. oc-rsync mirrors layer (1) verbatim in `transfer::setup::restrictions::apply_protocol_restrictions` and covers layer (2) in `ParsedServerFlags::clear_unsupported_features` plus the warning emission in `transfer::lib`. The new tests (`crates/transfer/tests/acl_negotiation_missing_remote.rs`) exercise the cross-layer behaviour end-to-end so future refactors cannot silently regress to either: - panicking when the peer advertises no ACL support, or - sending ACL bytes on the wire when the peer cannot decode them. Coverage: - Wire-level: server flag string without `A` yields acls=false. - Restriction layer: protocol 30+ does NOT reject --acls. - Restriction layer: protocol < 30 rejects with the exact upstream error message format byte-for-byte. - Restriction layer: local_server bypasses the protocol gate at any protocol (matching upstream's `!local_server` guard). - Cross-layer: protocol 32 + remote without `-A` results in graceful skip, no panic, no spurious warnings, no wire-level ACL data. - Ordering: when both gates fire (proto 29 + remote without -A), the protocol-version error wins, matching upstream's check order. - Idempotency: clear_unsupported_features must not double-report when the flag was already false from the wire. Findings: - oc-rsync matches upstream behaviour for the rejection path (byte-for-byte error message at restrictions.rs:96-101). - For the local-feature path, oc-rsync chooses graceful fallback (warn + continue) where upstream errors out - this is documented in flags.rs:144-146 and is the user-preferred behaviour. - Gate locations: crates/transfer/src/setup/restrictions.rs:96-101 (protocol gate) and crates/transfer/src/flags.rs:153 (local feature gate).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_6900e954aff08323888447dc61ec9260