refactor(mono): migrate integration tests to Qlean VM framework

[miliu2cc]

…remove SQLite dependency

[github-actions]

Claude finished @miliu2cc's task in 2m 36s —— View job

---

Code Review: Migrate Integration Tests to Qlean VM Framework

• Gather context and understand the PR
• Read CLAUDE.md for repo guidelines
• Review changed test files
• Analyze code quality, testing patterns, and correctness
• Post comprehensive review feedback

---

Summary: This PR replaces SQLite-backed unit tests with QEMU/KVM VM-based integration tests using the qlean crate. The approach is sound for realistic integration testing, but there are several issues worth addressing before merging.

---

Critical Issues

1. Massive Code Duplication Across All 5 Files

The following helper functions are duplicated verbatim (or nearly so) across every test file:

• exec_check — identical in all 5 files
• tracing_subscriber_init — identical in all 5 files
• install_docker — duplicated with minor inconsistencies
• upload_docker_compose — duplicated with minor inconsistencies
• retry_until — duplicated with subtle differences (see issue Fix the empty value in the dependabot.yml #2)

Recommendation: Extract these into a shared mono/tests/common/mod.rs module and use mod common; in each test file. This is the standard Rust pattern for shared test helpers. Fix this →

2. retry_until Inconsistency Between Files

buck_service_tests.rs's retry_until (line 92) lacks the last_error/last_output tracking that all other files include. When this function times out in buck_service_tests.rs, the error message contains far less diagnostic information than in the other files, making failures harder to debug.

// buck_service_tests.rs - less informative on timeout:
anyhow::bail!("{} did not become ready after {} seconds", service_name, max_retries * delay_secs);

// Other files - includes last error/output for diagnosis:
if let Some(err) = &last_error { msg.push_str(...) }
if let Some(output) = &last_output { msg.push_str(...) }

Fix this →

---

Code Quality Issues

3. Soft Test Failures (Tests That Warn Instead of Assert)

Multiple test phases accept unexpected HTTP status codes with tracing::warn! instead of failing the test. This can mask real regressions:

• buck_service_tests.rs Phase 1 (phase1_test_create_session): falls back to direct DB insertion on any non-200 response (401, 400, 500). A broken auth system would silently pass.
• buck_service_tests.rs Phase 2: logs warning on unexpected statuses rather than failing.
• login_user_extractor_tests.rs Phase 3 (phase3_test_invalid_cookie, line 546): warns when getting 200 for an invalid cookie — if auth isn't enforced, the test "passes".
• login_user_extractor_tests.rs Phase 4 (phase4_test_missing_cookie, lines 568–577): treats unknown status codes as warn! instead of bail!.

Tests that are "flaky" under normal conditions are better split into separate test phases or guarded by CI environment flags rather than silently downgrading assertions.

4. Duplicate Log Statement in buck_service_tests.rs

// Lines 183–186
async fn upload_docker_compose(vm: &mut qlean::Machine) -> Result<()> {
    tracing::info!("Uploading docker-compose.yml to VM...");         // ← stale/wrong name
    // Upload docker-compose.demo.yml to VM
    tracing::info!("Uploading docker-compose.demo.yml to VM...");    // ← correct name

The first log message is a leftover and should be removed. Fix this →

5. Inconsistent install_docker Implementation

buck_service_tests.rs (line 163) uses $(lsb_release -cs) to get the OS codename, while all other test files use $(. /etc/os-release && echo $VERSION_CODENAME). These produce identical results on Debian but indicate copy-paste inconsistency that could diverge over time.

---

Security Observations

6. Test Token Embedded in Git Remote URL (Visible in Logs)

In buck_service_tests.rs line 317:

exec_check(vm, "cd /tmp/mono && git remote add origin http://test_user:test-token-12345678-1234-1234-1234-123456789012@127.0.0.1:8000/.git 2>/dev/null || true").await?;

The hardcoded test token appears literally in the command string executed in the VM, which will be visible in verbose CI logs. While this is a test-only token (matching TEST_TOKEN const), consistent practice is to avoid embedding credentials directly in command strings — prefer using git credential helpers or environment variables even in tests.

7. Random ID Generation Collision Risk

In buck_service_tests.rs setup_test_users (line 264):

INSERT INTO access_token (id, ...) VALUES (floor(random() * 1000000000000)::bigint, ...)

Using a random BIGINT for a primary key risks (extremely rare) collisions on repeated test runs. Using a fixed test ID or PostgreSQL DEFAULT (if the column is a sequence/serial) would be more reliable.

---

Testing Coverage Concerns

8. Significant Regression in Unit-Level Coverage

The old SQLite-backed tests verified specific service-layer behaviors (wrong user, expired session, hash mismatch, etc.) at the unit level. The new tests exercise the same scenarios via HTTP but:

• Many assertions have been softened to warnings (see issue Updated dependabot.yml schedule to daily #3)
• DB-level state is verified by direct SQL queries rather than through service return values, so internal service logic is less thoroughly validated

It would be valuable to preserve some service-layer assertions alongside the VM integration tests, or ensure CI enforces strict assertion behavior (not just "no panic").

9. All Tests Are #[ignore] by Default

All new tests require --ignored to run. While this is appropriate given the QEMU/KVM dependencies, there should be CI documentation or a dedicated workflow job that runs these tests in an environment with the required dependencies. Otherwise it's hard to know if these tests are actually being run regularly.

---

Minor Nits

• campsite_api_store_tests.rs line 253: The Campsite API ECR image (mega:campsite-0.1.0-pre-release-amd64) is hardcoded without an env var override, unlike buck_service_tests.rs which correctly uses get_mega_ecr_image(). Fix this →
• session_management_tests.rs: Has a cleanup_docker function (line 232) but it is not called from the test function. On VM teardown this doesn't matter, but adding an explicit cleanup call makes the intent clearer.
• buck_service_tests.rs setup_postgres: Drops tables by name to handle schema drift (lines 242–251). This is fragile — if table names change in the schema, this cleanup becomes stale. A more robust approach is DROP SCHEMA public CASCADE; CREATE SCHEMA public; or using a test-specific schema.

---

Overall Assessment

The migration to Qlean VM-based integration tests is a meaningful improvement for realistic end-to-end coverage. The biggest concerns are:

1. The code duplication — this should be extracted to a common module before merging.
2. Soft assertions — several test scenarios should fail definitively rather than warn.
3. Missing CI configuration — it's unclear how/whether these #[ignore] tests are regularly executed in CI.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 65a239f71b

ℹ️ 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".

[chatgpt-codex-connector]

Send named session cookie in users/me requests

call_users_me formats the header as Cookie: {cookie}, but callers pass only raw values like valid_session_cookie; this does not produce a valid named session cookie (the app expects _campsite_api_session=<value>), so the “valid request” path is not actually exercising authenticated extraction and can pass/fail for the wrong reason.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Validate expired session before deleting test record

This phase deletes EXPIRED2 before issuing the API call that is supposed to verify expired-session behavior, so the request can only observe a missing session instead of an expired one; as a result, expiry validation is never truly tested and regressions in that path will be missed.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Initialize fallback repo with a work tree

On clone failure the fallback uses git init --bare, but later steps in the same flow run git add/git commit in /tmp/mono; because a bare repo has no working tree, bootstrap cannot create the initial commit and the monorepo setup silently degrades when this path is taken.

Useful? React with 👍 / 👎.

[genedna]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 65a239f71b

ℹ️ 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".

[chatgpt-codex-connector]

Keep expired session until validation request executes

The expired-session scenario is invalidated because EXPIRED2 is deleted before the API call that is supposed to verify expiry handling. In this order, the request tests a missing session path instead of an expired session path, so regressions in expiry logic can slip through while this phase still reports success.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Initialize fallback repo as non-bare working tree

When clone fails, the fallback runs git init --bare in /tmp/mono, but later steps write files and run git add/git commit in that same path. A bare repo has no working tree, so this fallback path cannot create the initial commit and breaks monorepo setup in the exact failure mode the fallback is meant to recover.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Validate rev-parse exit status before storing hashes

The code treats vm.exec success as if git rev-parse succeeded, but vm.exec can return Ok with a failing exit status; with 2>&1, failure text is captured into stdout and then written into mega_refs as commit/tree hashes. If HEAD is missing (e.g., after fallback init path), this stores invalid ref hashes and can cascade into misleading downstream API failures.

Useful? React with 👍 / 👎.

[Copilot]

Pull request overview

This PR refactors mono’s integration tests to run inside a Qlean-managed QEMU/KVM VM, aiming to remove reliance on the prior SQLite-based test_storage setup by standing up “real” services (Redis/MySQL/PostgreSQL and, for some tests, the Mono service) via Docker inside the VM.

Changes:

• Rewrote multiple mono/tests/* integration tests to boot a Qlean VM, install Docker, and start required services with docker compose.
• Replaced prior in-process/mocked tests (e.g., MemoryStore, mock Campsite API) with VM-level checks driven by curl / redis-cli / direct DB mutations.
• Expanded Buck service testing into a large multi-phase flow that provisions Postgres/Redis and launches the Mono service from an ECR image.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
mono/tests/session_management_tests.rs	Migrates session tests to VM + Docker Redis workflow.
mono/tests/http_server_session_tests.rs	Migrates HTTP session middleware tests to VM + Docker Redis workflow.
mono/tests/login_user_extractor_tests.rs	Migrates LoginUser extractor tests to VM + Docker (MySQL/Redis/Campsite API) and uses curl-based checks.
mono/tests/campsite_api_store_tests.rs	Migrates CampsiteApiStore tests to VM + Docker (MySQL/Campsite API) and uses direct endpoint checks.
mono/tests/buck_service_tests.rs	Replaces SQLite-backed service tests with VM + Docker Postgres/Redis + Mono service (ECR) + multi-phase API/DB checks.

[Copilot]

call_users_me builds the Cookie header as Cookie: <raw> (e.g. "valid_session_cookie"), which is not a valid cookie format and won't match what CampsiteApiStore/LoginUser expect (name=value, typically _campsite_api_session=<token>). This can make the "valid/invalid cookie" phases meaningless. Pass a proper cookie pair or omit the Cookie header entirely when no cookie is provided.

Suggested change

	let cmd = format!(
	"curl -s -w '\\nHTTP_CODE:%{{http_code}}' -H 'Cookie: {}' http://127.0.0.1:{}/v1/users/me",
	cookie, CAMPSITE_API_PORT
	// Build a proper Cookie header if a session token is provided; otherwise omit the header.
	let header_arg = if cookie.is_empty() {
	String::new()
	} else {
	format!("-H 'Cookie: _campsite_api_session={}'", cookie)
	};
	let cmd = format!(
	"curl -s -w '\\nHTTP_CODE:%{{http_code}}' {} http://127.0.0.1:{}/v1/users/me",
	header_arg, CAMPSITE_API_PORT

[github-actions]

Claude finished @miliu2cc's task in 2m 35s —— View job

---

Code Review: PR #1975 — Migrate Integration Tests to Qlean VM Framework

• Read CLAUDE.md for repo guidelines
• Review changed files
• Analyze code quality, testing patterns, and correctness
• Post comprehensive review feedback

---

Good progress since the last review. The second commit (8d9f4a4) successfully addressed the main code duplication concern by extracting shared helpers into mono/tests/common/mod.rs. Here are the remaining issues:

---

Critical Issues

1. git init --bare Cannot Support Working-Tree Operations (Buck Service)

mono/tests/buck_service_tests.rs line 92:

Err(e) => {
    tracing::warn!("Git clone error: {}, creating local bare repo", e);
    exec_check(vm, "mkdir -p /tmp/mono && cd /tmp/mono && git init --bare").await?;
}

A bare repository has no working tree, yet lines 97–103 unconditionally write a file to /tmp/mono/root.txt and run git add . && git commit. On the error recovery path, this sequence will always fail silently (the || true swallows it), leaving the monorepo uninitialized and all downstream test phases operating on a broken state.

Fix: Change git init --bare to git init on this path. Fix this →

---

2. Incomplete Extraction — Local Constants Still Shadow Common Module

The common module exports REDIS_CONTAINER, DOCKER_COMPOSE_FILE, etc., but individual test files still redefine them locally via use common::*. This means the common values are shadowed and the per-file redefinitions are dead code:

• session_management_tests.rs:38–39 — redefines REDIS_CONTAINER and DOCKER_COMPOSE_FILE
• http_server_session_tests.rs:40–41 — same
• login_user_extractor_tests.rs:42 — redefines DOCKER_COMPOSE_FILE
• campsite_api_store_tests.rs:46 — redefines DOCKER_COMPOSE_FILE
• buck_service_tests.rs:37–41 — redefines MEGA_HOST, MEGA_PORT, POSTGRES_USER, POSTGRES_DB

These local definitions also risk divergence if the common values are updated. They should be removed and the common constants relied upon directly. Fix this →

---

3. cleanup_docker Still Duplicated

The extraction missed two local cleanup_docker implementations:

• session_management_tests.rs:41–49
• http_server_session_tests.rs:43–51

Both are byte-for-byte identical and should live in common/mod.rs. Fix this →

---

4. setup_campsite_api Duplicated Between Two Test Files

The setup_campsite_api function is still fully duplicated between:

• login_user_extractor_tests.rs:48–173
• campsite_api_store_tests.rs:53–149

Additionally, these two duplicates pull different image tags:

// login_user_extractor_tests.rs:55
"docker pull public.ecr.aws/m8q5m4u3/mega:campsite-0.1.0-pre-release"
// campsite_api_store_tests.rs:59
"docker pull public.ecr.aws/m8q5m4u3/mega:campsite-0.1.0-pre-release-amd64"

Using different tags in tests that are supposed to test the same service is a latent correctness issue — one may be stale or for a different architecture. This function and the correct image reference should be consolidated in common/mod.rs with an env var override (like get_mega_ecr_image()). Fix this →

---

Code Quality Issues

5. Phase 2 of login_user_extractor_tests.rs Uses a Non-Existent Session

login_user_extractor_tests.rs:259:

let (status, body) = call_users_me(vm, "valid_session_cookie").await?;

call_users_me now correctly formats this as Cookie: _campsite_api_session=valid_session_cookie, but "valid_session_cookie" is a literal string with no corresponding record in the Campsite API's database. If the API properly enforces session validation, it will return 401 — and line 295 then calls anyhow::bail!("Expected 200 OK, got {}", status), failing the test.

The test passes only if Campsite API runs in a demo/bypass mode that ignores cookies. This means Phase 2 is not actually testing the "valid cookie" scenario it claims to test. Consider either inserting a real session into the DB before Phase 2, or marking this test as explicitly verifying demo-mode behavior.

---

6. git rev-parse Output Used as Hash Without Validating Exit Code

buck_service_tests.rs:121–143:

let commit_result = vm.exec("cd /tmp/mono && git rev-parse HEAD 2>&1").await;
let tree_result = vm.exec("cd /tmp/mono && git rev-parse HEAD^{tree} 2>&1").await;

if let (Ok(commit_ok), Ok(tree_ok)) = (commit_result, tree_result) {
    let commit_hash = String::from_utf8_lossy(&commit_ok.stdout).trim().to_string();
    // ... INSERT INTO mega_refs ...
}

vm.exec returns Ok even when the command exits with a non-zero code (e.g., when HEAD doesn't exist after the --bare fallback). The error text (e.g., "fatal: ambiguous argument 'HEAD': unknown revision") captured via 2>&1 would then be inserted as commit/tree hashes into mega_refs, silently poisoning downstream assertions. Check commit_ok.status.success() before using the output.

---

7. campsite_api_store_tests.rs Phase 5 — Inverted Success Predicate

campsite_api_store_tests.rs:307–318:

retry_until(
    vm,
    &format!("docker exec {} curl -sf -o /dev/null http://localhost:8080/health", CAMPSITE_API_CONTAINER),
    |output| output.is_empty(),   // ← success when output is empty?
    "Campsite API",
    60,
    2,
).await?;

curl -sf (with -f) returns exit code 22 on server errors; exec_check propagates the error when the exit code is non-zero. When the service is not ready, exec_check returns Err, triggering the error branch in retry_until. When the service IS ready, curl -sf -o /dev/null suppresses output and returns 0 — stdout is empty. So the predicate |output| output.is_empty() is actually correct for this specific curl invocation, but it's non-obvious and fragile (any future change to the curl flags could break it silently). Prefer |_output| true with a comment explaining that success is signaled by the command's exit code, or use -w '%{http_code}' and check for "200".

---

8. JSON Body Escaping in http_request is Fragile

buck_service_tests.rs:168:

cmd.push_str(&format!(" -d \"{}\"", body_data.replace('"', "\\\"")));

Shell-escaping JSON by simple quote substitution breaks on values containing backslashes or other special shell characters. Prefer using --data-raw with single-quoted strings or --data @- with heredoc-style input to avoid escaping issues.

---

Soft Assertion Concerns (Carried Over)

Phases 1, 2, 6, and 7 in buck_service_tests.rs still use tracing::warn! instead of anyhow::bail! for unexpected service responses. In a well-functioning CI environment these should be hard failures. The comments explain this is "acceptable in test env," but consider adding an environment flag (e.g., STRICT_ASSERTIONS=1) to opt into strict mode for CI runs that have the Mega service fully operational.

---

Overall Assessment

The common module extraction is a solid improvement. The main remaining items are:

1. The git init --bare bug — straightforward fix needed
2. Incomplete constant/function deduplication — three sets of identifiers are still duplicated
3. setup_campsite_api duplication with mismatched image tags — correctness risk
4. Phase 2 of login_user_extractor_tests.rs — the "valid cookie" test doesn't actually test a valid session

[github-actions]

Claude finished @miliu2cc's task in 3m 9s —— View job

---

Code Review: PR #1975 — Migrate Integration Tests to Qlean VM Framework

• Read CLAUDE.md for repo guidelines
• Review changed files (common/mod.rs, buck_service_tests.rs, login_user_extractor_tests.rs, campsite_api_store_tests.rs, session_management_tests.rs, cl_merge_integration.rs)
• Compare with previous review findings
• Analyze code quality, testing patterns, and correctness
• Post comprehensive review feedback

---

Summary of latest changes (commits 8d9f4a4 + 1c0191e): The author has addressed the top critical issue from the previous review by extracting shared helpers into mono/tests/common/mod.rs. This is a meaningful improvement. Several other issues remain and two new bugs were identified.

---

What Was Fixed ✅

1. Code duplication — All five shared helpers (exec_check, retry_until, install_docker, setup_redis, setup_postgres, setup_mysql, setup_mega_service, tracing_subscriber_init) are now centralized in mono/tests/common/mod.rs.
2. retry_until inconsistency — The unified version correctly tracks last_error and last_output for diagnostic output on timeout.
3. Duplicate log statement in install_docker (previous review item Welcome to mega Discussions! #4) — gone.
4. lsb_release vs /etc/os-release inconsistency — resolved by having a single shared install_docker.

---

Bugs That Must Be Fixed

1. git init --bare Fallback Creates Unusable Repository (buck_service_tests.rs:92)

The clone failure path creates a bare repo but immediately tries to use it as a working tree:

// Line 92 — creates bare repo (no working tree):
exec_check(vm, "mkdir -p /tmp/mono && cd /tmp/mono && git init --bare").await?;

// Lines 97–103 — writes a file and runs git add/commit (requires working tree):
vm.write(Path::new("/tmp/mono/root.txt"), b"Initial mono file").await?;
exec_check(vm, "cd /tmp/mono && git add . && git commit -m 'Initial commit' 2>/dev/null || true").await?;

git add and git commit in a bare repo silently fail because there is no working tree. The || true on line 101 masks this. Any downstream code that depends on the commit existing (e.g., the mega_refs insert at line 134) will store an empty or invalid hash. Fix this →

2. Unchecked git rev-parse Exit Status Stores Garbage Hashes (buck_service_tests.rs:121–131)

let commit_result = vm.exec("cd /tmp/mono && git rev-parse HEAD 2>&1").await;
let tree_result = vm.exec("cd /tmp/mono && git rev-parse HEAD^{tree} 2>&1").await;

if let (Ok(commit_ok), Ok(tree_ok)) = (commit_result, tree_result) {
    let commit_hash = String::from_utf8_lossy(&commit_ok.stdout).trim().to_string();
    // inserts commit_hash into mega_refs

vm.exec returns Ok(...) even when the shell command fails with non-zero exit — the Ok only means the RPC to the VM succeeded. git rev-parse HEAD failing (e.g., HEAD is unborn after the bare-repo bug) outputs fatal: ... to stderr, which with 2>&1 gets captured into stdout, and then stored as the "commit hash" in mega_refs. Neither commit_ok.status.success() nor tree_ok.status.success() is checked. Fix this →

---

Code Quality Issues

3. Common Constants Re-declared in Test Files (Shadows use common::*)

After the refactor, several test files re-declare constants that now live in common/mod.rs with identical values. These local declarations shadow the common imports, defeating the purpose of centralizing them and creating future drift risk:

File	Re-declared constants (already in common)
buck_service_tests.rs:37–41	MEGA_HOST, MEGA_PORT, POSTGRES_USER, POSTGRES_DB
cl_merge_integration.rs:53–70	POSTGRES_USER, POSTGRES_DB, POSTGRES_CONTAINER, DOCKER_COMPOSE_FILE
session_management_tests.rs:38–39	REDIS_CONTAINER, DOCKER_COMPOSE_FILE
login_user_extractor_tests.rs:43	DOCKER_COMPOSE_FILE

These should be removed and replaced with the common::* imported values. Fix this →

4. Campsite API Image Tag Inconsistency Between Test Files

login_user_extractor_tests.rs:55 pulls:

public.ecr.aws/m8q5m4u3/mega:campsite-0.1.0-pre-release

campsite_api_store_tests.rs:59 pulls:

public.ecr.aws/m8q5m4u3/mega:campsite-0.1.0-pre-release-amd64

These are different image tags. One of them may not exist in the registry. Both should use the same tag (likely -amd64 for consistency with MEGA_ECR_IMAGE_DEFAULT), or ideally both should use the same get_campsite_ecr_image() helper function with an env var override as was suggested in the previous review.

5. Redundant Duplicate DB Update in Phase 8 (buck_service_tests.rs:744–778)

The UPDATE buck_session SET status = 'manifest_uploaded' runs twice in the success path (once inside the status == 200 block at line 744, and unconditionally again at line 774). This is harmless but indicates a merge of fallback logic that wasn't cleaned up.

---

Soft Assertion Patterns (Unchanged from Previous Review)

The following remain and still mask potential regressions:

• buck_service_tests.rs Phase 1 (phase1_test_create_session): Any non-200 response (including 401, 400, 500) silently falls back to a direct DB insert. A completely broken auth system would pass this test.
• buck_service_tests.rs Phase 2, 6, 7: Unexpected HTTP status codes log warn! instead of failing.
• login_user_extractor_tests.rs Phase 3 (phase3_test_invalid_cookie): Getting 200 for an invalid cookie is accepted as "demo mode allows any cookie" with just a warning.
• login_user_extractor_tests.rs Phase 4 (phase4_test_missing_cookie): Any status code outside 200/401 logs warn! instead of bail!.

---

Security Observation (Unchanged)

buck_service_tests.rs:107 — the hardcoded test token appears verbatim in a git remote add shell command string that will appear in CI logs:

exec_check(vm, "cd /tmp/mono && git remote add origin http://test_user:test-token-12345678-1234-1234-1234-123456789012@127.0.0.1:8000/.git ...

Note that clone_url (containing the same token) is already constructed at line 75 and used at line 79 — the remote add on line 107 could simply reuse &clone_url instead of duplicating the literal.

---

Minor Nits

• campsite_api_store_tests.rs has no cleanup_docker call at the end of its test (unlike session_management_tests.rs and login_user_extractor_tests.rs). Not functionally required (VM teardown handles it), but inconsistent.
• Random BIGINT primary key for access_token inserts (buck_service_tests.rs:54, cl_merge_integration.rs:84,90) still risks rare collision. Using a fixed test ID would be more deterministic.
• CI documentation for running these #[ignore] tests still appears to be missing.

---

Overall Assessment

The refactoring to common/mod.rs is a clear improvement that addresses the main structural concern from the previous review. The two bugs (bare repo fallback and unchecked git rev-parse exit status) are the most pressing issues to fix before merging. The redundant constant declarations are a quick cleanup that should also be done to complete the refactor properly.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1c0191e4e0

ℹ️ 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".

[chatgpt-codex-connector]

Fail phase1 when session start API returns non-200

phase1_test_create_session falls back to direct DB insertion for 400, 500, and other unexpected statuses instead of failing the test. In those cases the suite can report success even if /api/v1/buck/session/start is broken, so regressions in authenticated session creation are silently masked rather than detected.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Reject 200 responses with non-JSON users/me payload

phase2_test_valid_request only validates the body inside if let Some(json) = body and has no failure path when parsing fails, so a 200 response with malformed or empty payload still passes this phase. That can hide API contract regressions for /v1/users/me while the integration test reports success.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Assert 404 for nonexistent Campsite endpoint

This phase logs non-404 results but still returns success, so it does not actually enforce the expected behavior for unknown routes. If /v1/nonexistent/endpoint starts returning 200/500, the test still passes and no regression is surfaced.

Useful? React with 👍 / 👎.

[github-actions]

Claude finished @miliu2cc's task in 3m 25s —— View job

---

Code Review: PR #1975 — Migrate Integration Tests to Qlean VM Framework

• Read CLAUDE.md for repo guidelines
• Review changed files (latest commit state: 710112e)
• Compare with previous review findings
• Analyze code quality, testing patterns, and correctness
• Post comprehensive review feedback

---

Good progress across all four commits. The git init --bare bug and unchecked git rev-parse exit status have been fixed, and the common module structure is now solid. Here is what remains.

---

Bugs Still Present

1. setup_campsite_api Pulls the Wrong Image (common/mod.rs:519)

pub async fn setup_campsite_api(vm: &mut qlean::Machine) -> Result<()> {
    tracing::info!("Pulling Campsite API image...");
    let ecr_image = get_mega_ecr_image();  // ← returns mono-0.1.0-pre-release-amd64
    exec_check(vm, &format!("docker pull {}", ecr_image)).await?;

get_mega_ecr_image() returns the mono service image. The log says "Pulling Campsite API image..." but it is actually pulling the mono binary image. docker compose up -d campsite_api that follows will use whatever image is defined in docker-compose.demo.yml, so the explicit pull wastes bandwidth on the wrong image. A CAMPSITE_ECR_IMAGE_DEFAULT constant and get_campsite_ecr_image() function are needed here, as was recommended in the first review cycle. Fix this →

---

Code Quality Issues

2. login_user_extractor_tests.rs:41–44 Still Shadows Common Constants

// login_user_extractor_tests.rs lines 41-44 — identical values already in common/mod.rs:25-26
const CAMPSITE_API_CONTAINER: &str = "mega-demo-campsite-api";
const CAMPSITE_API_PORT: u16 = 8080;

These shadow the common::* import with identical values. They should be removed. Fix this →

3. cl_merge_integration.rs:458–474 Hardcodes Container/DB Names

phase35_verify_cl1_merge directly strings mega-demo-postgres, mega, and mono instead of using the POSTGRES_CONTAINER, POSTGRES_USER, and POSTGRES_DB constants from common:

// line 458 — should use POSTGRES_CONTAINER, POSTGRES_USER, POSTGRES_DB
"docker exec mega-demo-postgres psql -U mega -d mono -t -c ..."

Similarly at line 463 and lines 477-484. Fix this →

4. update_cl_status Ignores the API Response (cl_merge_integration.rs:192)

async fn update_cl_status(...) -> Result<()> {
    exec_check(vm, &format!("curl -s -X POST ... -d '{{\"status\":\"{}\"}}'", token, cl, status)).await?;
    Ok(())
}

The API response is silently discarded. If the CL does not exist, auth fails, or the API returns an error, subsequent phases will operate on stale state with no indication why. The response should be checked for a success status code. Fix this →

---

Soft Assertion Patterns (Unchanged)

Several phases still use tracing::warn! where anyhow::bail! is appropriate:

5. phase35_verify_cl1_merge — Content Mismatch is a Warning, Not a Failure (cl_merge_integration.rs:517–523)

} else if !common_ok {
    tracing::warn!("  common.txt content unexpected: {}", common_content.trim()); // ← should bail!
} else if !file1_ok {
    tracing::warn!("  repo_a/file1.txt content unexpected: {}", file1_content.trim()); // ← should bail!
}

File content verification after a merge should be a hard assertion. Fix this →

6. Phase 7 — Unexpected Rebase Success Is Only a Warning (cl_merge_integration.rs:592)

if success {
    tracing::warn!("  Rebase succeeded (unexpected)");  // ← should be bail!
}

If the conflict-detection path is broken, this phase passes silently.

7. login_user_extractor_tests.rs Phase 4 — Unknown Status Codes Are Warnings (login_user_extractor_tests.rs:209–211)

_ => {
    tracing::warn!("  Unexpected status: {}", status);  // ← should be bail!
}
Ok(())

Any status code outside 200/401 (e.g., 500) causes the test to pass with a warning. Fix this →

---

Minor Nits

• cl_merge_integration.rs setup_test_users (lines 71, 77) — floor(random() * 1000000000000)::bigint for the primary key still risks (rare) collision. A fixed test ID (e.g. 1001, 1002) is more deterministic.
• cl_merge_integration.rs verify_conflict_record (line 279) — silently returns Ok(()) when the conversation response JSON fails to parse, rather than logging the raw response body and failing. This means a broken API that returns garbage JSON would pass the verification step.
• buck_service_tests.rs Phase 9 file upload — vm.exec is used directly instead of http_request_auth for the upload command, breaking the consistency of the file. The status code is also parsed by splitting lines rather than using the established http_request helper.
• CI documentation — Tests are all #[ignore]. There is still no documented workflow or CI job that runs them with the required QEMU/KVM dependencies.

---

What Was Fixed ✅ in This Round

1. git init --bare → git init — Both the Ok and Err fallback branches in init_monorepo now correctly create a working-tree repository.
2. git rev-parse exit status checked — commit_ok.status.success() and tree_ok.status.success() are now validated before inserting hashes into mega_refs.
3. setup_campsite_api extracted to common — No longer duplicated across test files.
4. retry_until predicate for campsite health — Fixed from |output| output.is_empty() to |output| output.trim() == "200" in campsite_api_store_tests.rs.
5. JSON body escaping — http_request now uses --data-raw with single-quoted body instead of manual replace('"', "\\\"").

---

Overall

The structural refactoring is now in good shape. The remaining work is:

1. The wrong ECR image in setup_campsite_api — correctness issue
2. The remaining duplicate constants in login_user_extractor_tests.rs
3. The hardcoded strings in cl_merge_integration.rs:phase35
4. Three soft assertions that should be hard failures

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 710112ec5e

ℹ️ 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".

[chatgpt-codex-connector]

Exercise middleware instead of writing Redis keys directly

This phase uses redis-cli SET/GET directly, so it never drives the HTTP session middleware path it claims to validate. If cookie handling, session serialization, or middleware wiring regresses in the server, this test can still pass because it only proves raw Redis operations work, which creates false confidence for a production-facing auth/session path.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Fail already-uploaded phase when upload returns 200

In the "already uploaded" scenario, a successful 200 response only emits a warning and the function still returns success. This lets the suite pass even if /api/v1/buck/session/{id}/file wrongly accepts re-uploads, so the regression this phase is intended to catch is silently missed.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Enforce network-error phase when request still succeeds

After stopping the Campsite API container, a successful request path is treated as a warning and the test continues. In stop-race or misconfigured environments this phase can pass without ever observing a connection failure, so network-error handling regressions are not reliably detected.

Useful? React with 👍 / 👎.