Conversation
# Conflicts: # docker-compose.yml
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdjusts Docker Compose Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as Agent
participant DryRun as "mongorestore (dry-run)"
participant Parser as "extract_db_name"
participant Restore as "mongorestore (actual)"
Agent->>DryRun: run dry-run against archive (stderr captured)
DryRun-->>Agent: stderr with "archive prelude <db>..."
Agent->>Parser: extract_db_name(dry stderr)
Parser-->>Agent: source_db (optional)
Agent->>Restore: run actual mongorestore with --nsInclude/--nsFrom/--nsTo remapping from source_db -> cfg.database
Restore-->>Agent: success / error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 1 minute and 27 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/domain/postgres/restore.rs (1)
39-56:⚠️ Potential issue | 🟠 MajorDocument the database pre-existence requirement for PostgreSQL restore.
With
--createremoved and the connection URL pointing tocfg.database,pg_restorecannot create the database and will fail ifcfg.databasedoesn't already exist. Unlike MySQL/MariaDB which explicitlyCREATE DATABASEwithin their restore flow, PostgreSQL expects the target database to be provisioned beforehand. TheRestoreService.run_restore()will fail silently at the ping stage if the database is unreachable.Confirm that your external provisioning (operator action, migration, IaC, or service initialization) always creates
cfg.databasebeforerestore::run()is invoked, and add a doc comment to therun()function insrc/domain/postgres/restore.rsdocumenting this contract to prevent future connection errors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/postgres/restore.rs` around lines 39 - 56, The restore now calls pg_restore without --create and connects to cfg.database (see PostgresDumpFormat::Fc block using --dbname &url), so the target database must exist beforehand; add a clear doc-comment to the run() function (and/or RestoreService::run_restore()) in this file stating that cfg.database must be provisioned prior to invocation (operator/IaC/migration must create it), mention that pg_restore cannot create the DB when --create is removed, and note that callers should ensure provisioning to avoid silent ping failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docker-compose.yml`:
- Around line 12-13: Replace the developer-specific absolute host path
"/Users/charlesgauthereau/Desktop/test-portabase/databases.json" in the
docker-compose volume mapping with a portable alternative: either restore the
relative mount "./databases.json:/config/config.json" or parameterize via an env
var (e.g., use a DB_CONFIG variable) so contributors and CI can provide their
own host path instead of committing a machine-specific absolute path.
- Line 23: The commit leaks a sensitive EDGE_KEY value in docker-compose.yml
(the EDGE_KEY environment variable); rotate the exposed master key on the server
immediately and remove the hard-coded value from docker-compose.yml, then
reference a runtime variable (e.g., ${EDGE_KEY}) instead and load it from a
gitignored .env or docker-compose.override.yml kept out of source control;
update any deployment docs or dev README to show how to provision EDGE_KEY
locally and ensure docker-compose.yml no longer contains the literal key.
In `@src/domain/mariadb/backup.rs`:
- Around line 33-37: The code computes a version-specific path into the
mariadb_dump variable via select_mariadb_path(&version) but then calls
Command::new("mariadb-dump"), ignoring that path; replace the hardcoded
Command::new("mariadb-dump") with a call that uses the computed mariadb_dump
(e.g., pass mariadb_dump or mariadb_dump.as_os_str() / mariadb_dump.as_path() to
Command::new) so the spawned process uses the version-specific binary selected
by select_mariadb_path and matches the logged path.
In `@src/domain/postgres/restore.rs`:
- Around line 142-144: The FD branch is passing "--dbname={}" literally then a
separate &url which becomes a stray positional arg to pg_restore; update the
pg_restore argument construction so the DB URL is attached correctly — either
split into two .arg calls like .arg("--dbname").arg(&url) or build a single
formatted string via format!("--dbname={}", url); locate the FD branch around
the pg_restore invocation (the .arg("--dbname={}") and .arg(&url) calls near
dump_dir) and replace those two .arg calls accordingly so pg_restore receives
the intended --dbname value.
---
Outside diff comments:
In `@src/domain/postgres/restore.rs`:
- Around line 39-56: The restore now calls pg_restore without --create and
connects to cfg.database (see PostgresDumpFormat::Fc block using --dbname &url),
so the target database must exist beforehand; add a clear doc-comment to the
run() function (and/or RestoreService::run_restore()) in this file stating that
cfg.database must be provisioned prior to invocation (operator/IaC/migration
must create it), mention that pg_restore cannot create the DB when --create is
removed, and note that callers should ensure provisioning to avoid silent ping
failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 92bafc5b-79c0-4ca5-97f0-50dd308a9100
📒 Files selected for processing (6)
docker-compose.ymlsrc/domain/mariadb/backup.rssrc/domain/mariadb/restore.rssrc/domain/mysql/backup.rssrc/domain/mysql/restore.rssrc/domain/postgres/restore.rs
| LOG: debug | ||
| TZ: "Europe/Paris" | ||
| EDGE_KEY: "eyJzZXJ2ZXJVcmwiOiJodHRwOi8vbG9jYWxob3N0Ojg4ODciLCJhZ2VudElkIjoiNzM0NjU3Y2YtMGQzYy00Y2UwLTkyODQtZDJmOGYyMjI2MzgzIiwibWFzdGVyS2V5QjY0IjoiMUh0djdtWCtYVkJxL0IzUEV2WDlZZjlQeUdVZW5oRHlXemo5THRqNW90WT0ifQ==" | ||
| EDGE_KEY: "eyJzZXJ2ZXJVcmwiOiJodHRwOi8vbG9jYWxob3N0Ojg4ODciLCJhZ2VudElkIjoiNjIyNWFlMzMtODQwMy00NmE3LWEyNDEtMjU4MTI2MjVlYTA4IiwibWFzdGVyS2V5QjY0IjoiQlhWM1hvbEM2NTZTVjdkTmdjV1BHUWxrKytycExJNmxHRGk3Q1BCNWllbz0ifQ==" |
There was a problem hiding this comment.
Likely credential leak — rotate immediately.
The new EDGE_KEY base64-decodes to a JSON payload containing serverUrl, agentId, and masterKeyB64. Committing this value into a tracked file in a shared repo is equivalent to publishing the master key. Two actions are required:
- Rotate the master key on the server side and reissue a fresh
EDGE_KEYfor this agent — assume the current value is compromised the moment this PR is pushed. - Move the value out of
docker-compose.yml. Use a non-tracked.env(gitignored) or a developer-local override file (docker-compose.override.yml) and reference it via${EDGE_KEY}.
🔒 Proposed fix
- EDGE_KEY: "eyJzZXJ2ZXJVcmwi...=="
+ EDGE_KEY: ${EDGE_KEY:?EDGE_KEY must be provided via .env}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| EDGE_KEY: "eyJzZXJ2ZXJVcmwiOiJodHRwOi8vbG9jYWxob3N0Ojg4ODciLCJhZ2VudElkIjoiNjIyNWFlMzMtODQwMy00NmE3LWEyNDEtMjU4MTI2MjVlYTA4IiwibWFzdGVyS2V5QjY0IjoiQlhWM1hvbEM2NTZTVjdkTmdjV1BHUWxrKytycExJNmxHRGk3Q1BCNWllbz0ifQ==" | |
| EDGE_KEY: ${EDGE_KEY:?EDGE_KEY must be provided via .env} |
🧰 Tools
🪛 Betterleaks (1.1.2)
[high] 23-23: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 23-23: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docker-compose.yml` at line 23, The commit leaks a sensitive EDGE_KEY value
in docker-compose.yml (the EDGE_KEY environment variable); rotate the exposed
master key on the server immediately and remove the hard-coded value from
docker-compose.yml, then reference a runtime variable (e.g., ${EDGE_KEY})
instead and load it from a gitignored .env or docker-compose.override.yml kept
out of source control; update any deployment docs or dev README to show how to
provision EDGE_KEY locally and ensure docker-compose.yml no longer contains the
literal key.
| let mariadb_dump = select_mariadb_path(&version).join("mariadb-dump"); | ||
| info!("Mariadb dump found: {}", mariadb_dump.display()); | ||
|
|
||
|
|
||
| let output = Command::new("mariadb-dump") |
There was a problem hiding this comment.
mariadb_dump is resolved by version but never used — falls back to $PATH.
Lines 33–34 compute and log a version-specific path, then line 37 spawns the bare string "mariadb-dump", which makes select_mariadb_path effectively dead code and breaks the per-version targeting. If the host has multiple mariadb-dump binaries (or none on $PATH), behavior diverges from what the log line claims.
🔧 Proposed fix
- let output = Command::new("mariadb-dump")
+ let output = Command::new(&mariadb_dump)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let mariadb_dump = select_mariadb_path(&version).join("mariadb-dump"); | |
| info!("Mariadb dump found: {}", mariadb_dump.display()); | |
| let output = Command::new("mariadb-dump") | |
| let mariadb_dump = select_mariadb_path(&version).join("mariadb-dump"); | |
| info!("Mariadb dump found: {}", mariadb_dump.display()); | |
| let output = Command::new(&mariadb_dump) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/mariadb/backup.rs` around lines 33 - 37, The code computes a
version-specific path into the mariadb_dump variable via
select_mariadb_path(&version) but then calls Command::new("mariadb-dump"),
ignoring that path; replace the hardcoded Command::new("mariadb-dump") with a
call that uses the computed mariadb_dump (e.g., pass mariadb_dump or
mariadb_dump.as_os_str() / mariadb_dump.as_path() to Command::new) so the
spawned process uses the version-specific binary selected by select_mariadb_path
and matches the logged path.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/domain/mysql/restore.rs (1)
19-22: Backtick quoting helps, but consider validatingcfg.database.Wrapping the identifier in backticks fixes the common case (reserved words, hyphens). However, if
cfg.databaseever contains a backtick (e.g. from a misconfigured config file), the DROP/CREATE statement will break out of the quoted identifier. Since this string is also passed via-eto a privilegedmysqlsession, it's worth either validating the database name against an allow-list pattern (e.g.^[A-Za-z0-9_]+$) at config load time, or escaping embedded backticks by doubling them (`→``).Not a blocker given the value originates from trusted config, but the same hardening should apply symmetrically here and in
src/domain/mariadb/restore.rs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/domain/mysql/restore.rs` around lines 19 - 22, The DROP/CREATE SQL string built into drop_create_cmd using cfg.database is vulnerable if cfg.database contains backticks; either validate the database name against a tight allow-list regex like ^[A-Za-z0-9_]+$ (reject/return error at config load) or sanitize by escaping any backticks (replace ` with ``) before interpolation; apply the same fix symmetrically in src/domain/mariadb/restore.rs where the same pattern is used so both drop_create_cmd usages are safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/domain/mysql/restore.rs`:
- Around line 19-22: The DROP/CREATE SQL string built into drop_create_cmd using
cfg.database is vulnerable if cfg.database contains backticks; either validate
the database name against a tight allow-list regex like ^[A-Za-z0-9_]+$
(reject/return error at config load) or sanitize by escaping any backticks
(replace ` with ``) before interpolation; apply the same fix symmetrically in
src/domain/mariadb/restore.rs where the same pattern is used so both
drop_create_cmd usages are safe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 58cc19d0-03a9-447e-af02-23ed341ad333
📒 Files selected for processing (3)
src/domain/mariadb/restore.rssrc/domain/mysql/restore.rssrc/domain/postgres/restore.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/domain/postgres/restore.rs
- src/domain/mariadb/restore.rs
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/domain/mongodb/connection.rs`:
- Around line 34-45: The current logic collects candidate DB names into the
HashSet `dbs` and then uses `dbs.into_iter().next()` which is nondeterministic
when multiple source DBs exist; change this to explicitly check `dbs.len()` and
fail closed if it is not exactly 1 (e.g., return None or an Err) so a multi-DB
archive doesn't randomly pick a DB; replace the final `dbs.into_iter().next()`
with a branch that returns the single DB when `dbs.len() == 1` and returns an
error/None when `dbs.is_empty()` or `dbs.len() > 1`, referencing the `dbs`
variable and the parsing loop that finds `"archive prelude "` to locate the
code.
In `@src/domain/mongodb/restore.rs`:
- Around line 29-31: The dry-run subprocess result stored in dry_run must have
its exit status verified before attempting to parse stderr; after running the
dry-run (dry_run variable) check dry_run.status.success() and return an error
(with context like "mongorestore --dryRun failed") if it failed, so that
failures (auth/network/archive) are reported instead of being masked by
extract_db_name(&dry_output) parsing errors; update the logic around the dry_run
handling to mirror the later restore output.status.success() check and only call
extract_db_name when the dry_run succeeded.
- Around line 15-22: The dry-run Command construction currently in the block
that defines dry_run and builds a "--uri=..." string should use the same logic
as get_mongo_uri() to avoid inserting empty "username:password" and
"authSource=admin" for no-auth configs; update the dry-run URI construction (the
Command::new(&mongorestore) invocation that sets .arg(format!("--uri={}", ...)))
to call or reimplement the get_mongo_uri() logic (checking
cfg.username/cfg.password and conditionally including credentials and
authSource) so the dry-run URI matches the actual restore URI returned by
get_mongo_uri().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 86d42ed2-5f99-41b5-a385-df711a27b781
📒 Files selected for processing (5)
justfilesrc/domain/mongodb/backup.rssrc/domain/mongodb/connection.rssrc/domain/mongodb/restore.rssrc/services/status.rs
💤 Files with no reviewable changes (2)
- src/services/status.rs
- src/domain/mongodb/backup.rs
| let mut dbs = std::collections::HashSet::new(); | ||
| for line in dry_output.lines() { | ||
| if let Some(pos) = line.find("archive prelude ") { | ||
| let rest = &line[pos + "archive prelude ".len()..]; | ||
| if let Some(dot) = rest.find('.') { | ||
| let db = &rest[..dot]; | ||
| dbs.insert(db.to_string()); | ||
| } | ||
| } | ||
| } | ||
| dbs.into_iter().next() | ||
| } |
There was a problem hiding this comment.
Fail closed when multiple source DBs are detected.
HashSet::into_iter().next() at Line 44 is nondeterministic for multi-DB archives, so restore target mapping can randomly pick the wrong source DB.
💡 Proposed fix
pub fn extract_db_name(dry_output: &str) -> Option<String> {
- let mut dbs = std::collections::HashSet::new();
+ let mut dbs = std::collections::BTreeSet::new();
for line in dry_output.lines() {
if let Some(pos) = line.find("archive prelude ") {
let rest = &line[pos + "archive prelude ".len()..];
if let Some(dot) = rest.find('.') {
let db = &rest[..dot];
- dbs.insert(db.to_string());
+ if !db.is_empty() {
+ dbs.insert(db.to_string());
+ }
}
}
}
- dbs.into_iter().next()
+ if dbs.len() == 1 {
+ dbs.into_iter().next()
+ } else {
+ None
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/mongodb/connection.rs` around lines 34 - 45, The current logic
collects candidate DB names into the HashSet `dbs` and then uses
`dbs.into_iter().next()` which is nondeterministic when multiple source DBs
exist; change this to explicitly check `dbs.len()` and fail closed if it is not
exactly 1 (e.g., return None or an Err) so a multi-DB archive doesn't randomly
pick a DB; replace the final `dbs.into_iter().next()` with a branch that returns
the single DB when `dbs.len() == 1` and returns an error/None when
`dbs.is_empty()` or `dbs.len() > 1`, referencing the `dbs` variable and the
parsing loop that finds `"archive prelude "` to locate the code.
| let dry_run = Command::new(&mongorestore) | ||
| .arg(format!( | ||
| "--uri={}", | ||
| format!( | ||
| "mongodb://{}:{}@{}:{}/?authSource=admin", | ||
| cfg.username, cfg.password, cfg.host, cfg.port | ||
| ) | ||
| )) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For mongorestore, is URI mongodb://:@host:port/?authSource=adminvalid when authentication is disabled, or should anonymous connections usemongodb://host:port/?
💡 Result:
When MongoDB authentication is truly disabled, you should use an anonymous connection string that does not include username/password material. So, use: mongodb://host:port/ The URI you proposed, mongodb://:@host:port/?authSource=admin, is not the right form for an anonymous (no-credentials) connection. In MongoDB connection strings, the "username:password@" portion is optional authentication credentials; the client only attempts authentication when credentials are provided. The authSource option is defined as the database associated with user credentials, and MongoDB ignores authSource when no username is provided. Given that, providing empty credentials while also specifying authSource is at best unnecessary and may be rejected or mis-parsed by drivers. Recommended for mongorestore (no auth): mongorestore --uri "mongodb://host:port/" ./dump If you want to restore a specific database, include it in the path (without auth options), e.g.: mongorestore --uri "mongodb://host:port/testdb" ./dump
Citations:
- 1: https://www.mongodb.com/docs/current/reference/connection-string-formats/
- 2: https://www.mongodb.com/docs/manual/reference/connection-string-options/
- 3: https://www.mongodb.com/docs/database-tools/mongorestore
🏁 Script executed:
cat -n src/domain/mongodb/restore.rs | head -50Repository: Portabase/agent
Length of output: 2471
🏁 Script executed:
cat -n src/domain/mongodb/connection.rsRepository: Portabase/agent
Length of output: 1832
Use consistent URI construction for no-auth MongoDB configs in dry-run command.
At lines 15–22, the dry-run command always injects username:password and authSource=admin into the URI. For no-auth configurations (empty username or password), this produces an invalid URI like mongodb://:@host:port/?authSource=admin. Meanwhile, the actual restore command at line 36 uses get_mongo_uri(), which correctly handles no-auth by returning mongodb://host:port/database. This inconsistency causes dry-run to fail on valid no-auth setups while the actual restore would succeed.
Apply the same conditional logic from get_mongo_uri() to construct the dry-run URI:
Proposed fix
- let dry_run = Command::new(&mongorestore)
+ let dry_uri = if cfg.username.is_empty() || cfg.password.is_empty() {
+ format!("mongodb://{}:{}/", cfg.host, cfg.port)
+ } else {
+ format!(
+ "mongodb://{}:{}@{}:{}/?authSource=admin",
+ cfg.username, cfg.password, cfg.host, cfg.port
+ )
+ };
+
+ let dry_run = Command::new(&mongorestore)
.arg(format!(
"--uri={}",
- format!(
- "mongodb://{}:{}@{}:{}/?authSource=admin",
- cfg.username, cfg.password, cfg.host, cfg.port
- )
+ dry_uri
))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let dry_run = Command::new(&mongorestore) | |
| .arg(format!( | |
| "--uri={}", | |
| format!( | |
| "mongodb://{}:{}@{}:{}/?authSource=admin", | |
| cfg.username, cfg.password, cfg.host, cfg.port | |
| ) | |
| )) | |
| let dry_uri = if cfg.username.is_empty() || cfg.password.is_empty() { | |
| format!("mongodb://{}:{}/", cfg.host, cfg.port) | |
| } else { | |
| format!( | |
| "mongodb://{}:{}@{}:{}/?authSource=admin", | |
| cfg.username, cfg.password, cfg.host, cfg.port | |
| ) | |
| }; | |
| let dry_run = Command::new(&mongorestore) | |
| .arg(format!( | |
| "--uri={}", | |
| dry_uri | |
| )) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/mongodb/restore.rs` around lines 15 - 22, The dry-run Command
construction currently in the block that defines dry_run and builds a
"--uri=..." string should use the same logic as get_mongo_uri() to avoid
inserting empty "username:password" and "authSource=admin" for no-auth configs;
update the dry-run URI construction (the Command::new(&mongorestore) invocation
that sets .arg(format!("--uri={}", ...))) to call or reimplement the
get_mongo_uri() logic (checking cfg.username/cfg.password and conditionally
including credentials and authSource) so the dry-run URI matches the actual
restore URI returned by get_mongo_uri().
| let dry_output = String::from_utf8_lossy(&dry_run.stderr); | ||
| let source_db = extract_db_name(&dry_output) | ||
| .context("Could not detect source database name from archive")?; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In mongorestore --dryRun --verbose mode, are archive prelude details guaranteed on stderr only, and should callers treat non-zero exit as authoritative failure before parsing output?
💡 Result:
Non-zero exit status: treat it as authoritative failure. Don’t base correctness on whether archive “prelude” details appear on stderr vs stdout. stderr vs stdout for archive prelude details - MongoDB’s docs state that the database tools (including mongorestore) print logs to stderr. - The documentation for --dryRun + --verbose describes verbosity affecting “internal reporting returned on standard output or in log files” (i.e., you should not assume anything about where specific messages land). - In the mongorestore code, archive prelude metadata is parsed/used internally (and failures in parsing/reading return an error), rather than being something a caller should infer from stderr-only output. Should callers parse output and ignore exit code? - No: callers should key off mongorestore’s process exit status. If mongorestore encounters errors such as inability to parse/validate options or read archive/prelude metadata, the tool returns an error result and exits non-zero. - In general, callers should not treat “message seen on stderr” as the only signal. Use the exit code to decide whether it is safe to parse any summary output. Practical guidance 1) Run mongorestore with --dryRun --verbose and capture both streams if you need logs. 2) If the command’s exit code is non-zero, treat the run as failed and do not trust/parse success summaries that may still have been printed. 3) Only parse success/summary output when exit code is 0. Grounding notes: This applies to archive prelude handling because prelude metadata is not merely informational; mongorestore reads it as part of establishing the archive/dump format and may abort the restore when it can’t be processed, independent of whether any particular detail was emitted on stderr.
Citations:
- 1: https://www.mongodb.com/docs/database-tools/mongorestore/mongorestore-behavior-access-usage/
- 2: https://docs.mongodb.org/manual/reference/program/mongorestore/
- 3: https://github.com/mongodb/mongo-tools/blob/master/mongorestore/mongorestore.go
- 4: https://mongodbcom-cdn.staging.corp.mongodb.com/docs/database-tools/logs/
🏁 Script executed:
cat -n src/domain/mongodb/restore.rs | sed -n '1,50p'Repository: Portabase/agent
Length of output: 2471
🏁 Script executed:
wc -l src/domain/mongodb/restore.rsRepository: Portabase/agent
Length of output: 92
🏁 Script executed:
cat -n src/domain/mongodb/restore.rs | sed -n '15,45p'Repository: Portabase/agent
Length of output: 1501
Check dry-run exit status before parsing stderr.
The dry-run command exits at line 27 without verifying success. If mongorestore --dryRun fails due to authentication, network, or archive issues, line 30 currently masks the real error by reporting a parse failure instead. Add a status check immediately after line 27:
Fix
let dry_output = String::from_utf8_lossy(&dry_run.stderr);
+ if !dry_run.status.success() {
+ anyhow::bail!("mongorestore dry-run failed for {}: {}", cfg.name, dry_output);
+ }
let source_db = extract_db_name(&dry_output)
.context("Could not detect source database name from archive")?;The actual restore command at line 46 correctly checks output.status.success() — apply the same pattern here.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let dry_output = String::from_utf8_lossy(&dry_run.stderr); | |
| let source_db = extract_db_name(&dry_output) | |
| .context("Could not detect source database name from archive")?; | |
| let dry_output = String::from_utf8_lossy(&dry_run.stderr); | |
| if !dry_run.status.success() { | |
| anyhow::bail!("mongorestore dry-run failed for {}: {}", cfg.name, dry_output); | |
| } | |
| let source_db = extract_db_name(&dry_output) | |
| .context("Could not detect source database name from archive")?; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/domain/mongodb/restore.rs` around lines 29 - 31, The dry-run subprocess
result stored in dry_run must have its exit status verified before attempting to
parse stderr; after running the dry-run (dry_run variable) check
dry_run.status.success() and return an error (with context like "mongorestore
--dryRun failed") if it failed, so that failures (auth/network/archive) are
reported instead of being masked by extract_db_name(&dry_output) parsing errors;
update the logic around the dry_run handling to mirror the later restore
output.status.success() check and only call extract_db_name when the dry_run
succeeded.
1 participant
Summary by CodeRabbit
Bug Fixes
Chores
Other