fix: thirdparty repo import ref inconsistency#2042
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.
Reviewed commit: b31fa7a865
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| command.default_branch = true; | ||
| default_exist = true; | ||
| } |
There was a problem hiding this comment.
When no default branch exists, this block sets command.default_branch = true, but branch refs are no longer persisted in-loop. Because the handler was constructed earlier from commands.clone(), ImportRepo::finalize_receive_pack later writes branch refs from a stale command_list snapshot where default_branch is still false. On first push to an import repo, this can leave import_refs without any default branch, which breaks head selection paths that rely on default_branch (for example refs_with_head_hash returning a zero head).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9bcfcfead
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| tracing_subscriber::fmt() | ||
| .with_writer(file_appender) | ||
| .with_max_level(log_level) | ||
| .with_ansi(config.with_ansi) |
There was a problem hiding this comment.
When print_std is false, logs are written only to the rolling file appender, but this branch still applies .with_ansi(config.with_ansi). Because the new config default sets with_ansi = true, production/file-only deployments will emit ANSI escape sequences into log files, which can break downstream log parsing/ingestion and conflicts with the comment that this mode should be non-ANSI.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR addresses third-party import inconsistencies and correctness issues around ref persistence during receive-pack flows, while also improving operational logging (per-request trace IDs) and simplifying dependency boundaries (routing common third-party crates via jupiter).
Changes:
- Make import-repo attach + ref updates transactional and concurrency-safe (CAS on monorepo root ref with retry), and align mono/import receive-pack finalize flow.
- Centralize shared third-party crates behind
jupiterre-exports (e.g.,sea_orm,idgenerator,redis) and move email implementation intomono. - Update logging configuration: remove configurable
log_path, addwith_ansi, and introduce per-request trace context middleware + span enrichment.
Reviewed changes
Copilot reviewed 44 out of 45 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| vault/src/nostr.rs | Switch nostr key generation RNG to OsRng. |
| vault/Cargo.toml | Reorder/add deps; adds rand feature flags. |
| mono/tests/common/mod.rs | Update test config snippet for new log config (with_ansi). |
| mono/src/server/trace_context.rs | New middleware/utilities for request trace id propagation and span fields. |
| mono/src/server/mod.rs | Export trace context module and helpers. |
| mono/src/server/http_server.rs | Wire trace middleware + TraceLayer span customization; update CORS headers. |
| mono/src/notification/triggers.rs | Route SeaORM imports through jupiter::sea_orm. |
| mono/src/notification/dispatcher.rs | Route mailer via crate::email; route SeaORM error via jupiter::sea_orm. |
| mono/src/main.rs | Export email module from the binary crate. |
| mono/src/lib.rs | Export email module from the library crate. |
| mono/src/email/mod.rs | Move email module to depend on common config/errors. |
| mono/src/cli.rs | Change log initialization: fixed log dir, stdout-only vs file-only, add with_ansi. |
| mono/src/api/router/webhook_router.rs | Route idgenerator + ActiveEnum via jupiter re-exports. |
| mono/src/api/router/bot_router.rs | Route SeaORM datetime type via jupiter::sea_orm. |
| mono/Cargo.toml | Add/adjust deps (e.g., lettre, uuid), drop direct sea-orm/idgenerator deps. |
| jupiter/src/storage/mono_storage.rs | Add get_ref_by_name_in_txn; add CAS-based attach helper for monorepo root ref update. |
| jupiter/src/storage/mod.rs | Add begin_db_transaction helper on shared connection. |
| jupiter/src/storage/git_db_storage.rs | Add *_in_txn ref mutation helpers for import refs. |
| jupiter/src/storage/bots_storage.rs | Update hmac imports for new version (KeyInit). |
| jupiter/src/service/webhook_service.rs | Update hmac imports for new version (KeyInit). |
| jupiter/src/redis/mod.rs | Re-export redis types; disambiguate external redis crate paths. |
| jupiter/src/lib.rs | Re-export idgenerator and sea_orm for downstream crates. |
| jupiter/Cargo.toml | Update features/deps (tokio/uuid/rand/reqwest). |
| jupiter/callisto/Cargo.toml | Remove unused serde_json dependency. |
| docs/development.md | Update config env-var examples for new log fields and examples. |
| context/src/lib.rs | Consume redis types via jupiter::redis re-exports. |
| context/Cargo.toml | Drop direct redis dependency. |
| config/config.toml | Remove log_path, adjust defaults, add with_ansi. |
| common/src/lib.rs | Remove email module export from common. |
| common/src/errors.rs | Add StaleMonorepoRootRef error for CAS retry flow. |
| common/src/config/mod.rs | Remove log_path from LogConfig; add with_ansi. |
| common/Cargo.toml | Drop email-related deps; keep sea-orm directly in common (cycles). |
| ceres/src/protocol/smart.rs | Move branch ref persistence to finalize stage; sync command list; adjust status reporting. |
| ceres/src/protocol/mod.rs | Store handler command list behind Mutex for post-unpack synchronization. |
| ceres/src/pack/monorepo.rs | Persist mono branch CL refs in a single transaction; rename post-receive hook to finalize. |
| ceres/src/pack/mod.rs | Replace post_receive_pack with finalize_receive_pack; add sync_commands_after_unpack. |
| ceres/src/pack/import_repo.rs | Make attach transactional with retries; persist branch import_refs in txn; tag-only immediate updates. |
| ceres/src/api_service/mono_api_service.rs | Remove now-redundant save_import_ref helper. |
| ceres/src/api_service/commit_ops.rs | Route redis async commands via jupiter::redis. |
| ceres/src/api_service/cache.rs | Route redis types via jupiter::redis. |
| ceres/src/api_service/blob_ops.rs | Remove unused hex import after dependency updates. |
| ceres/src/api_service/admin_ops.rs | Route redis async commands via jupiter::redis. |
| ceres/Cargo.toml | Reorder/adjust deps; remove direct redis; add explicit feature flags. |
| Cargo.toml | Bump key dependency versions (SeaORM, sha1/sha2, hmac). |
| Cargo.lock | Lockfile updates for dependency bumps. |
| // File logs always use the standard Mega cache directory (not configurable). | ||
| let file_appender = tracing_appender::rolling::hourly(mega_cache().join("logs"), "mono-logs"); | ||
|
|
||
| if config.print_std { | ||
| let stdout = std::io::stdout; | ||
| // Local development: log only to stdout, ANSI controlled by config. | ||
| tracing_subscriber::fmt() | ||
| .with_writer(stdout.and(file_appender)) | ||
| .with_writer(std::io::stdout) |
There was a problem hiding this comment.
file_appender is created unconditionally, but when print_std is true the subscriber is configured to log only to stdout, so the comment “File logs always …” is misleading and the file appender is unused. Consider either logging to both stdout+file (as before) or updating the comment and avoiding creating the file appender in the stdout-only branch.
| // Production / non-stdout: log only to file, without ANSI so GCP/AWS can parse cleanly. | ||
| tracing_subscriber::fmt() | ||
| .with_writer(file_appender) | ||
| .with_max_level(log_level) | ||
| .with_ansi(config.with_ansi) | ||
| .init(); |
There was a problem hiding this comment.
The comment says the non-stdout path logs “without ANSI”, and LogConfig docs say with_ansi has no effect on file logs, but the code still calls .with_ansi(config.with_ansi) for file logging. Either force ANSI off for file logs (and keep with_ansi for stdout only) or adjust the comments/docs to match the actual behavior.
| pub log_path: PathBuf, | ||
| pub level: String, | ||
| pub print_std: bool, | ||
| /// Whether to enable ANSI colors for stdout logs (has no effect on file logs). |
There was a problem hiding this comment.
with_ansi is documented as “has no effect on file logs”, but mono::cli::init_log currently applies it to the file subscriber as well. Either enforce the “stdout-only” semantics in the logger setup, or update this field’s doc comment to reflect that it also controls ANSI for file logs.
| /// Whether to enable ANSI colors for stdout logs (has no effect on file logs). | |
| /// Whether to enable ANSI colors for logs. | |
| /// | |
| /// This setting is applied by the logger configuration and may affect both | |
| /// stdout and file log output. |
| let storage = self.storage.git_db_storage(); | ||
| match refs.command_type { | ||
| CommandType::Create => { | ||
| storage | ||
| .save_ref(self.repo.repo_id, refs.clone().into()) | ||
| .await | ||
| .unwrap(); | ||
| .map_err(|e| GitError::CustomError(e.to_string()))?; | ||
| } | ||
| CommandType::Delete => storage | ||
| .remove_ref(self.repo.repo_id, &refs.ref_name) | ||
| .await | ||
| .unwrap(), | ||
| .map_err(|e| GitError::CustomError(e.to_string()))?, | ||
| CommandType::Update => { | ||
| storage | ||
| .update_ref(self.repo.repo_id, &refs.ref_name, &refs.new_id) | ||
| .await | ||
| .unwrap(); | ||
| .map_err(|e| GitError::CustomError(e.to_string()))?; | ||
| } | ||
| } |
There was a problem hiding this comment.
These tag ref operations now propagate errors instead of unwrap(), but GitDbStorage::update_ref (called in the CommandType::Update case) still uses unwrap().unwrap() internally and can panic if the ref row is missing. Consider switching tag updates to the safe update path (like update_ref_in_txn does) or refactoring GitDbStorage::update_ref to return a proper error instead of panicking.
| let new_commit = Commit::from_tree_id( | ||
| save_trees | ||
| .back() | ||
| .ok_or_else(|| MegaError::Other("no tree generated".to_string()))? | ||
| .id, | ||
| vec![ObjectHash::from_str(&expected_commit).unwrap()], |
There was a problem hiding this comment.
ObjectHash::from_str(&expected_commit).unwrap() can panic on malformed data (e.g., DB corruption or unexpected root ref contents), taking down the receive-pack request. Prefer converting this into a MegaError with context so the operation fails gracefully instead of panicking.
| let new_commit = Commit::from_tree_id( | |
| save_trees | |
| .back() | |
| .ok_or_else(|| MegaError::Other("no tree generated".to_string()))? | |
| .id, | |
| vec![ObjectHash::from_str(&expected_commit).unwrap()], | |
| let expected_commit_hash = ObjectHash::from_str(&expected_commit).map_err(|err| { | |
| MegaError::Other(format!( | |
| "invalid root ref commit hash '{}': {}", | |
| expected_commit, err | |
| )) | |
| })?; | |
| let new_commit = Commit::from_tree_id( | |
| save_trees | |
| .back() | |
| .ok_or_else(|| MegaError::Other("no tree generated".to_string()))? | |
| .id, | |
| vec![expected_commit_hash], |
| /// Resolve trace id from headers or generate a new one. | ||
| pub fn resolve_trace_id(headers: &HeaderMap) -> Arc<str> { | ||
| for name in ["x-request-id", "x-trace-id"] { | ||
| if let Some(raw) = headers.get(name).and_then(|v| v.to_str().ok()) { | ||
| let s = raw.trim(); | ||
| if is_valid_inbound_trace_token(s) { | ||
| return Arc::from(s.to_string()); | ||
| } | ||
| } | ||
| } | ||
| Arc::from(Uuid::new_v4().to_string()) | ||
| } | ||
|
|
||
| fn is_valid_inbound_trace_token(s: &str) -> bool { | ||
| !s.is_empty() | ||
| && s.len() <= MAX_INBOUND_LEN | ||
| && s.bytes().all(|b| { | ||
| matches!(b, b'a'..=b'z' | b'A'..=b'Z' | b'0'..=b'9' | b'-' | b'_' | b'.' | b':' | b'/' ) | ||
| }) | ||
| } |
There was a problem hiding this comment.
New trace-id parsing/validation logic is introduced here (header precedence, token validation, max length, response echo), but there are no unit tests to lock in the expected behavior. Since mono/src/server/http_server.rs already has tests, consider adding focused tests for resolve_trace_id/is_valid_inbound_trace_token (valid tokens, invalid chars, overlong values, whitespace trimming, and fallback UUID generation).
| async-trait = { workspace = true } | ||
| hex = { workspace = true } | ||
| pgp = { workspace = true } | ||
| rand = { workspace = true, features = ["thread_rng"] } | ||
| serde = { workspace = true, features = ["derive"] } | ||
| serde_json = { workspace = true } |
There was a problem hiding this comment.
rand is used in this crate, but there are no thread_rng call sites in vault/src (the updated code uses OsRng and rand::random). Consider removing the explicit features = ["thread_rng"] to avoid enabling unnecessary features and reduce the chance of feature-mismatch across workspace crates.
| bytes = { workspace = true } | ||
| async-trait = { workspace = true } | ||
| hex = { workspace = true } | ||
| rand = { workspace = true, features = ["thread_rng"] } |
There was a problem hiding this comment.
Same as vault: this enables rand’s thread_rng feature, but there are no thread_rng usages in ceres/src (search shows none). Consider dropping the feature flag unless it’s required transitively, to keep workspace dependency features minimal and consistent.
| rand = { workspace = true, features = ["thread_rng"] } | |
| rand = { workspace = true } |
| hmac = { workspace = true } | ||
| hex = { workspace = true } | ||
| rand = { workspace = true } | ||
| rand = { workspace = true, features = ["thread_rng"] } |
There was a problem hiding this comment.
Same as vault/ceres: features = ["thread_rng"] is enabled, but there are no thread_rng call sites in jupiter/src (search shows none). Consider removing it unless there’s a specific reason to force-enable that feature at the crate level.
| rand = { workspace = true, features = ["thread_rng"] } | |
| rand = { workspace = true } |
No description provided.