[codex] Resolve review findings and stabilize gates#188
Conversation
Registry Notary has no dependency on the manifest crates; only Relay consumes registry-manifest-core. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
A tag-push run and a manual dispatch for the same tag could race and clobber each other's published assets. Key the concurrency group on workflow+ref with cancel-in-progress: false so a second run queues instead of cancelling a publish mid-flight. Security review notes: release-provenance surface. No publish steps change, only run serialization; cancellation stays disabled because image push, asset upload with --clobber, cosign signing, and SLSA provenance are not safely cancellable. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
The quick start still described the pre-monorepo standalone repo: cloning jeremi/registry-lab with submodules, just quick as the entry point, and a vendor-submodule pin workflow for Platform/Relay/Notary/ Manifest that no longer exists. Describe the monorepo checkout and just release-fast instead, keeping sibling-checkout overrides as a labeled alternative. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
[workspace.lints.rust] unsafe_code = "forbid" was declared but no member crate had [lints] workspace = true, so it was enforced nowhere. Opt in the 23 members with no unsafe code; the five that use unsafe (libyaml FFI, libc setrlimit, env::set_var in test support) carry a comment explaining why they stay out. Security review notes: hardens the unsafe-code posture only; no code or dependency changes. Verified by a temporary unsafe probe failing compilation in an opted-in crate, then a clean workspace check. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
The OpenAPI 200 example for /admin/v1/posture lacked the deployment and audit sections the server emits at the default tier, and nothing gated example fidelity. Add the missing sections, pin the example's top-level keys to the live document in a standalone_http test for both tiers, and regenerate the committed OpenAPI artifact (which also catches info.version up from 0.6.2 to 0.8.3). Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
The root gate ran only fmt, a workspace check, and registryctl tests; OpenAPI drift and dependency policy were manual discipline and drifted repeatedly. Gate the full workspace test suite, cargo deny check for bans/licenses/sources, the notary openapi-check recipe, and the relay openapi-contract recipe, with a build cache to keep runtime sane. Clippy and the cargo-deny advisories section are deliberately not yet gated: the tree currently fails them (4 pre-existing lints; fresh RUSTSEC advisories on anyhow and quick-xml). They will be promoted once fixed rather than silenced. Security review notes: both new actions (taiki-e/install-action, Swatinem/rust-cache) are pinned to the same full commit SHAs already trusted in the per-product workflows; tools are version-pinned (cargo-deny 0.19.8, just 1.51.0). No deny.toml ignores added. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
cargo clippy --workspace --all-targets -- -D warnings is the house convention but the tree had drifted: is_none_or over map_or(true, ..), a field reassign after Default::default(), a derivable Default impl, two too_many_arguments sites (annotated, matching the 48 existing allow sites), three needless borrows, and a useless format! in a test fixture. All behavior-preserving; relay, notary-core, notary-server, and notary test suites pass. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
Lockfile-only bump to 1.0.103. The two quick-xml advisories (RUSTSEC-2026-0194/0195) remain open: calamine 0.35 pins ^0.39 and phonenumber pins <=0.38 under the pinned Crosswalk dependency, so no upgrade path exists until upstream moves; no deny.toml ignore added. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
The workspace is clippy-clean under -D warnings as of the preceding lint fixes, so gate it. The cargo-deny advisories section stays local: the remaining RUSTSEC advisories (quick-xml 0194/0195) have no upgrade path until calamine and phonenumber move upstream. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
![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.
Reviewed commit: ebc9325ca8
ℹ️ 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".
Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c180c2c424
ℹ️ 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".
| default_manifest_src := if path_exists("../registry-manifest/Cargo.toml") == "true" { "../registry-manifest" } else { if path_exists("../../registry-manifest/Cargo.toml") == "true" { "../../registry-manifest" } else { "./vendor/registry-manifest" } } | ||
| default_stack_src := if path_exists("../Cargo.toml") == "true" { if path_exists("../crates/registry-relay/Cargo.toml") == "true" { ".." } else { "" } } else { "" } | ||
| default_relay_src := if default_stack_src != "" { "../crates/registry-relay" } else { if path_exists("../registry-relay/Cargo.toml") == "true" { "../registry-relay" } else { if path_exists("../../registry-relay/Cargo.toml") == "true" { "../../registry-relay" } else { "./vendor/registry-relay" } } } | ||
| default_notary_src := if default_stack_src != "" { default_stack_src } else { if path_exists("../registry-notary/Cargo.toml") == "true" { "../registry-notary" } else { if path_exists("../../registry-notary/Cargo.toml") == "true" { "../../registry-notary" } else { "./vendor/registry-notary" } } } |
There was a problem hiding this comment.
In a clean monorepo checkout this new default exports REGISTRY_NOTARY_SOURCE_DIR=.., but lab/scripts/smoke-notary-client.py only looks for the Python client at ${REGISTRY_NOTARY_SOURCE_DIR}/bindings/python before falling back to sibling/vendor checkouts. The monorepo client actually lives under products/notary/bindings/python, so just notary-client (and the release check step that invokes the same smoke) still exits with “Registry Notary Python client was not found” unless a separate sibling checkout or manual REGISTRY_NOTARY_CLIENT_SOURCE_DIR is present. Fresh evidence beyond the earlier vendor-default comment is that the new monorepo default now points at a real root that still lacks the expected bindings/python layout.
Useful? React with 👍 / 👎.
| `scripts/check-release-source-model.sh source` compares sibling Platform, | ||
| Relay, and Notary SHAs with the Lab `vendor/` pins and fails on mismatches or | ||
| dirty source checkouts. Use `REGISTRY_LAB_ALLOW_PENDING_PINS=1` only while the | ||
| final source commits are still waiting for the Lab submodule pin update. | ||
| `scripts/check-release-source-model.sh vendor` proves that the selected release | ||
| paths resolve to committed Lab pins. | ||
| Relay, and Notary SHAs with the Lab `vendor/` pins (Crosswalk, registry-atlas, | ||
| and the eSignet Relay authenticator) and fails on mismatches or dirty source |
There was a problem hiding this comment.
For users following this documented source proof with sibling Platform/Relay/Notary checkouts, the validator still calls compare_pin against lab/vendor/registry-platform, lab/vendor/registry-relay, and lab/vendor/registry-notary, but this monorepo change leaves only Crosswalk/atlas/eSignet under lab/vendor. Even with all sibling source env vars set and REGISTRY_LAB_ALLOW_PENDING_PINS=1, scripts/check-release-source-model.sh source fails when it tries to read the removed vendor dirs, so this source-proof path is no longer runnable as described.
Useful? React with 👍 / 👎.
1 participant
Summary
This PR carries the reviewed local
mainchanges plus cleanup commits that resolve the remaining actionable findings from the review pass and the follow-up PR review feedback.What changed
cargo denyback to fullcargo deny check.deny.tomlignores forRUSTSEC-2026-0194andRUSTSEC-2026-0195, with review triggers for upstream quick-xml movement and untrusted XLSX/XML inputs.Content-Lengthinstead of racing a socket upload.sha256fields from the Notary posture example and regenerated OpenAPI sample, with a regression assertion for Notary posture artifacts..gitmodules, makingjust setupinitializelab/vendor/*paths from the superproject root, and defaulting Lab source variables to the monorepo source paths.Why
The quick-xml advisories currently have no direct upgrade path in this workspace because calamine and the pinned Crosswalk/phonenumber stack still constrain quick-xml. The gate should still run in CI, so the risk is accepted narrowly and review-triggered instead of leaving advisories out of CI.
The two flaky tests were passing in isolation but could fail under full workspace runs because one depended on network upload timing and the other assumed aggregate row ordering from DataFusion.
The Notary posture public example had drifted from the live response shape by retaining nested artifact hashes the current code no longer emits.
The Lab README documented a clean monorepo quick start, but the justfile and nested
.gitmoduleslayout still reflected the old vendor/sibling checkout model.Validation
Ran locally before publishing and during review follow-up:
cargo fmt --checkcargo check --locked --workspace --all-targetscargo clippy --workspace --all-targets -- -D warningscargo test --locked --workspacecargo deny checkcd products/notary && just openapi-checkcd crates/registry-relay && just openapi-contractpython3 -m unittest lab/scripts/test_esignet_relay_lab.py lab/scripts/test_check_release_source_model.pycd lab && scripts/check-release-source-model.sh monorepocd lab && just --summarygit diff --checkRemaining note: Relay is still not opted into the workspace lints table because its public test-support/env-mutation path needs a separate focused cleanup.