chore(notary): follow-up fixes from the #345 review#349
Conversation
#345 split the notary monoliths into module trees, invalidating the file:line and file::symbol citations in the release evidence docs and product specs. Point every citation at the code's new home; each one was verified to resolve against the current tree. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
strip_rust_test_module only handled inline #[cfg(test)] mod blocks, so test modules that #345 moved into #[path]-included files or include!-d shards were scanned as production code by route-sources. Resolve #[cfg(test)] module declarations to the files they compile (expanding include! transitively) and skip those files during route extraction. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
#345 rewrote the healthcheck and sidecar upstream mocks from .route to .fallback so the route-sources check would not see them in the split-out test files. The checker now strips external test modules, so restore the explicit routes as they were before #345. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
The modularization defaulted the split-out auth helpers to pub(in super::super). Most are used only within their own file or within standalone::auth: 10 become private and 5 pub(super); the rest genuinely serve the standalone-level test module and stay. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
…directly The correlation-id functions reached their callers through a three-hop re-export chain (compat.rs, standalone/runtime.rs, the standalone.rs facade). Delete the shim and the re-exports; every call site now uses crate::request_context directly. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
The config test shards each opened with #[allow(unused_imports)] and a wildcard glob over all sibling modules, hiding what each shard actually uses and muting the compiler. Import the used items explicitly. 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: 607208a69b
ℹ️ 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".
| for include in INCLUDE_MACRO_RE.finditer(body): | ||
| target = (current.parent / include.group(1)).resolve() | ||
| if target not in excluded: | ||
| excluded.add(target) | ||
| pending.append(target) |
There was a problem hiding this comment.
When the excluded #[cfg(test)] file is a normal module that declares child modules with mod child; (for example, src/api/tests/mod.rs already uses this layout), those child files are compiled only under the cfg(test) parent, but this traversal only follows include!. Any Axum mock route added in such a child will still be scanned as a production route source and make check_security_assurance.py route-sources fail even though it is test-only; expand regular child modules from excluded files too.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in ca026f9: the transitive expansion from excluded test files now follows plain mod child; declarations (with or without #[path]) as well as include!, so children of a #[cfg(test)] mod.rs are excluded too. Covered by a new failing-first test mirroring the api/tests/mod.rs layout.
Codex review on #349: the test-module exclusion expanded include! references transitively but not plain `mod child;` declarations, so a child module of a #[cfg(test)] mod.rs (the layout api/tests/mod.rs uses) would still be scanned as a production route source. Expand child module declarations from excluded files too; everything a test-only file declares is test-only. Signed-off-by: Jeremi Joslin <jeremi@joslin.fr>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Context
#345 (the Registry Notary modularization) merged while its review was still in flight. The review confirmed the refactor is a semantic no-op in production code, and surfaced 7 non-blocking findings. This PR carries all 7.
Changes
release/notes/unsafe-code-inventory.md: refreshed every file:line citation (the old ones pointed past EOF of the emptied monoliths), plus a handful of line numbers that had drifted independently of refactor(notary): modularize Registry Notary #345.release/notes/negative-path-coverage-map.md: retargeted the 31 stalefile::symbolcitations to the test shards the symbols moved into.check_security_assurance.py:strip_rust_test_moduleonly handled inline#[cfg(test)] modblocks, so test modules that refactor(notary): modularize Registry Notary #345 moved into#[path]-included files orinclude!-d shards were scanned as production code. The route-sources scan now resolves#[cfg(test)]module declarations to the files they compile (expandinginclude!transitively) and excludes them. Covered by 3 new unit tests, written failing-first. The healthcheck and sidecar mocks that refactor(notary): modularize Registry Notary #345 rewrote from.routeto.fallbackto dodge the old checker are restored to their pre-refactor(notary): modularize Registry Notary #345 form.standalone/auth/{oidc,credentials,notary_tokens}.rsdefaulted topub(in super::super); 10 are now private and 5pub(super), with the remainder genuinely required by the standalone-level test module.scalability-spec.md+citizen-self-attestation-spec.md: refreshed the stale line/symbol cross-references (e.g.api.rs::evaluateis nowapi/evaluations.rs::evaluate).#[allow(unused_imports)]wildcard sibling globs with explicit imports in all 7 shards.standalone/compat.rs: deleted the three-hop correlation-id re-export chain (compat.rs, runtime.rs, the standalone facade); every call site importscrate::request_contextdirectly.Verification
cargo check --locked --workspace --all-targets: clean, zero warningscargo test --locked -p registry-notary-core: 365 passed;-p registry-notary-server --lib: 318 passed; healthcheck tests 8/8; sidecar fail-closed test 1/1check_security_assurance.pymodes from nightly-security.yml green for both notary and relay, with the restored.routemocks in placeNote for later
The
release/evidence docs andproducts/notary/specs/cite source by file:line with no validator, so any large refactor silently invalidates them. A CI check for citation freshness would prevent a repeat; intentionally not included here.