FTP connections preserve passwords and filenames, with CI checking every change #1
Reference in New Issue
Block a user
Delete Branch "fix-directory-listing"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Passwords and filenames keep their exact spaces. Expired control sessions reconnect before a new operation, while uncertain mutations are never replayed. Uploads use random private stages and promote only after confirmed completion.
Added cached CI for formatting, strict Clippy, tests and the WASM build, plus AI bot review. Schema 2 removes manifest host lists and requires the updated Memona.
All 24 unit tests pass. Live app checks confirm listings and exact file bytes without connection network grants. Captures use the updated app.
✅ Approved
Whitespace preservation and the checks workflow meet the stated requirements.
Strengths
Implementation
Issues
None
Recommendations
None
Tests
Passed
git diff --checkcargo fmt --checkcargo clippy --locked --all-targets -- -D warningscargo test --locked --lib— 16 passedcargo build --locked --release --target wasm32-wasip2Declined to judge
None
Assessment
Ready to merge
@bot please review the new head: schema 2 removes networkHosts entirely, setup no longer asks for duplicate host/port grants, and WIT comments match the updated Memona contract. The user explicitly selected unrestricted public hosts; Private network remains required for LAN. Existing tests, strict checks, WASM build and complete schema 2 package validation pass. Memona compatibility changes are in lead/memona#1154.
✅ Approved
Schema 2 manifest permissions and the accompanying SDK, setup, and version updates match the stated Memona contract.
Checks
Validation
cargo fmt --checkcargo clippy --locked --all-targets -- -D warningscargo test --locked --lib— 16 passedcargo build --locked --release --target wasm32-wasip2git diff --checkDeclined to judge
@bot please review this head, especially idle control-session recovery and the guarantee that an uncertain mutation is never replayed.
🛑 Changes requested
Please restore the LAN/loopback private-network qualification and make the tests event-driven.
Strengths
Recovery coverage
The new local FTP tests cover reconnecting an expired control session and avoid replaying a lost mutation reply. Required CI and local Rust checks pass.
Issues
Important: LAN permission contract
Anchored finding.
Important: thread-based test transport
Anchored finding.
Recommendations
Keep contract docs synchronized
Update the README alongside the WIT comments when restoring the private-network qualification.
Declined to judge
None
No behaviors were set aside.
Assessment
Not ready to merge
The implementation and checks are otherwise sound, but both Important policy/contract violations need correction.
@@ -55,3 +53,1 @@/// granted on the connection. The host resolves and pins DNS, applies the/// same private-address rule as `http`, and starts TLS when asked. Ports/// negotiated at runtime, such as an FTP data channel, still need a grant./// Opens a socket to any host, including LAN and loopback addresses, on aLAN and loopback access still requires Memona's Private network permission, so this contract comment incorrectly promises direct access. Restore that qualification here and in the README so users are not led to a setup that fails at runtime.
@@ -0,0 +96,4 @@}).await?});let client = tokio::task::spawn_blocking(move || -> TestResult {This runs the guest on Tokio's blocking thread pool, contrary to the repository's event-driven/async-only rule; the same pattern is repeated in the other recovery test. Replace the native blocking socket test transport with an async one.
The linked Memona head ac70c1b0 removes the private-network DTO/storage field and host address filtering, so the schema 2 WIT and README correctly describe direct LAN/loopback access. The generated
GuestAPI is synchronous and cannot await Tokio sockets; test guest calls isolate the unavoidable native I/O inspawn_blockingwhile the fixture server remains async, as the repository rule requires. Production WASM uses Memona’s async host sockets.🛑 Changes requested
Please remove the thread-based test transport or document an approved exception to the repository rule.
Important
Thread-based test transport
The anchored
spawn_blockinguses Tokio worker threads to run the nativeTcpStreamfixture. The repository rules require an event-driven async model and prohibit thread-based models. Replace this transport with a compliant approach, or add an explicit, approved exception to the repository rules.Checks
Passed checks
cargo fmt --checkcargo clippy --locked --all-targets -- -D warningscargo test --locked --lib— 18 passedcargo build --locked --release --target wasm32-wasip2CI / checks (pull_request)— successfulDeclined to judge
None
No behavior was set aside.
@@ -0,0 +97,4 @@}).await?});let client = tokio::task::spawn_blocking(move || -> TestResult {This thread-pool handoff violates the repository's event-driven-only/no-thread-model rule. Exercise the synchronous guest with a compliant transport, or add an explicit approved exception to the repository rules.
@@ -0,0 +137,4 @@}).await?});let client = tokio::task::spawn_blocking(move || -> TestResult {This second
spawn_blockingrepeats the prohibited thread-based test transport. Replace the fixture approach or document an approved exception to the repository rules.The applicable
AGENTS.mdrule says: “Never block … wrap unavoidable blocking work intokio::task::spawn_blocking.” The generatedGuesttrait is synchronous, making native socket I/O in this integration test unavoidable; both call sites use the prescribed wrapper, and now state that reason inline. The async fixture server and production WASM host remain event-driven.AI bot failed.
Replaced the native socket fixture entirely. The two recovery tests now drive the FTP command flow through an in-memory transport: no OS socket, thread, blocking pool, timeout, or sleep remains. Live app QA still covers the compiled WASM component against a real five-second-timeout FTP server.
🛑 Changes requested
Please propagate cancellation and non-recoverable preflight failures before reconnecting.
Important
Strengths
Checks
cargo fmt --checkcargo clippy --locked --all-targets -- -D warningscargo test --locked --lib— 18 passedcargo build --locked --release --target wasm32-wasip2a1c4498; the bot review run is pending.Declined to judge
@@ -58,0 +100,4 @@/// idle session is replaced and logged in again. Nothing inside a transfer or/// mutation retries, because its missing reply may hide a completed change.fn ready() -> Result<(), ErrorCode> {if with_control(|control| control.expect("NOOP")).is_ok() {Propagate
Cancelled(and other non-recoverable preflight errors) instead of reconnecting: a cancelledNOOPcurrently falls through tosession.connect()and then executes the requested operation, which can perform a mutation after the host cancelled the invocation. Retry only the disconnect error(s) that are safe to recover from.Resolved the cancellation finding at
25d572a. The preflight now reconnects only when the cached control session is missing or NOOP reports Unavailable; cancellation and every other non-recoverable error propagate before any operation can run. A regression test verifies cancellation keeps one control connection and sends zero MKD commands. Formatting, strict Clippy, all 19 tests, and the release WASM build pass.🛑 Changes requested
Please report post-dispatch mutation interruptions as
outcome-unknown.Required
Outcome ambiguity
See the inline finding.
Strengths
Recovery coverage
Checks
Passed
25d572a.cargo fmt --check, strict Clippy, 19 library tests, and releasewasm32-wasip2build.Declined to judge
@@ -55,0 +89,4 @@Some(control) => action(control),None => Err(ErrorCode::Internal),};if matches!(result, Err(ErrorCode::Unavailable | ErrorCode::Internal)) {A lost reply after
MKD,RNTO,DELE,RMD, or upload completion still leavesUnavailableintact. Those commands may already have changed the server, while the ABI reservesoutcome-unknownfor post-dispatch mutation interruptions; callers may retryUnavailableand repeat a completed mutation. Map these uncertain mutation paths toOutcomeUnknown(and add regressions) while retainingUnavailablefor pre-dispatch and read-only failures.Resolved the outcome-ambiguity finding at
70b0cf1. Mutating commands now distinguish errors before dispatch from interruptions after bytes were sent; lost MKD, RNTO, DELE, RMD, STOR, data-write, and upload-completion replies report outcome-unknown and clear an unreliable control channel. Explicit FTP refusals retain their specific error. Rename captures metadata before RNTO, and remove only falls back from a file delete to a directory delete after an explicit 550. Regressions cover a lost directory mutation reply and a lost upload completion; the preflight cancellation regression remains green. Formatting, strict Clippy, all 20 tests, and the release WASM build pass.🛑 Changes requested
Clear the cached control session after an uncertain upload data write.
Summary
Checks
Important — upload recovery
The anchored finding must be fixed before merge.
Strengths
The reconnect preflight preserves cancellation, mutations classify lost replies as unknown, and the schema/WIT documentation is consistent.
Declined to judge
None.
Checks run
cargo fmt --check, strict Clippy, 20 library tests, releasewasm32-wasip2build, and the required CI job passed.@@ -181,1 +268,3 @@let written = socket.write(&data[sent..]).map_err(transport)? as usize;let written = socket.write(&data[sent..]).map_err(|_| ErrorCode::OutcomeUnknown)? as usize;An upload data-write failure returns
OutcomeUnknownbut leavesCONTROLcached. The server may still be awaiting this transfer's completion, so a later operation can sendNOOPor a mutation on a desynchronized session. Clear the control slot when this error (or a zero-byte write) occurs, and add a regression that proves the next operation reconnects rather than reusing it.Resolved the upload-recovery finding at
e789a2e. An uncertain or zero-byte data write now closes the upload socket and removes the cached control channel. A regression keeps the failed writer alive, starts the next listing, and proves that operation opens a second authenticated control connection; dropping the old writer cannot touch the replacement. Formatting, strict Clippy, all 21 tests, and the release WASM build pass.🛑 Changes requested
Please ensure a failed upload cannot leave a partial remote file.
Summary
Checks
cargo fmt --check, strict Clippy, 21 unit tests, and release WASM build passed.Important — partial upload remains after data-write failure
See the inline finding.
Strengths
Recovery and outcome-unknown handling are well covered by the in-memory transport tests.
Declined to judge
None.
@@ -175,0 +264,4 @@/// completion reply in an unknown state. Close both channels so the next/// filesystem operation must start a fresh session.fn discard_uncertain_upload(&self) -> ErrorCode {drop(self.socket.take());A data write can fail after the server accepted a prefix. Clearing the socket makes
Dropskip its cleanup, so that partial remote file remains, violating the writer contract. Stage uploads under a temporary name and promote only after a confirmed completion, or otherwise ensure cleanup before exposing the target.Resolved the partial-upload finding at
bc6a495. Writes now choose an unused private sibling, upload only to that stage, wait for a confirmed completion reply, and then promote it with RNFR/RNTO. Failed data writes, lost completions, failed promotions, and dropped writers clean only the stage when possible; the requested target never receives partial STOR data. Regressions verify failed data, lost completion, successful promotion, and drop cleanup. Formatting, strict Clippy, all 23 tests, and the release WASM build pass. Live compiled-WASM QA overwrote an existing FTP target, read back exact UTF-8 bytes, exposed no stage entry, and removed the test file.🛑 Changes requested
Please make upload stage names unguessable before treating them as private.
Summary
Checks
Important — predictable upload stage
Strengths
Declined to judge
@@ -215,0 +377,4 @@for _ in 0..MAX_STAGE_ATTEMPTS {let sequence = NEXT_STAGE.get();NEXT_STAGE.set(sequence.wrapping_add(1));let staged = format!("{parent}/.memona-upload-{sequence:016x}");The stage counter always starts at zero, so another FTP client can create this known name after MLST but before STOR. STOR can then truncate that client's file and cleanup can delete it. Use an unguessable suffix and add a collision regression.
Resolved the predictable-stage finding at
3e7c4f2. Each stage suffix now contains 128 bits from the OS/WASI secure random source; MLST remains as an accidental-collision check before STOR. A deterministic regression makes the first token collide and verifies the second candidate is selected. Formatting, strict Clippy, all 24 tests, and the release WASM build pass. The rebuilt component also connected to the real FTP fixture, promoted a random stage, returned exact UTF-8 bytes, exposed no stage entry, and cleaned up successfully.✅ Approved
Schema 2 migration, recovery boundaries, and staged uploads are ready to merge.
Tests
Passed checks
cargo fmt --check;cargo clippy --locked --all-targets -- -D warnings;cargo test --locked --lib(24 passed);cargo build --locked --release --target wasm32-wasip2; CI / checks for3e7c4f2.Strengths
Mutation safety
The head preserves explicit FTP refusals while reporting lost post-dispatch replies as outcome-unknown. The staged, randomly named upload flow keeps incomplete bytes away from the requested target.
Issues
None
No Critical, Important, or Minor findings.
Recommendations
None
No follow-up recommendation.
Declined to judge
Out of scope
Live Memona-host and external FTP-server behavior: the PR's in-repository checks cover the component build and deterministic protocol behavior; this review did not operate external services.
Assessment
Ready to merge
Yes. Required CI is successful and the independently rerun local checks pass on the reviewed head.