fix(tier-2): CodeRabbit follow-ups from PR #100

scripts/test-consistency.sh

@@ -545,11 +545,14 @@ assert_both "TOUCH missing" TOUCH edge:nomiss
 # ===========================================================================
 log "=== SWAPDB ==="
 
-# Seed: db0 has swapkey=hello, db1 is empty
-both SELECT 0
-both SET swapkey hello
-both SELECT 1
-both DEL swapkey
+# Seed: db0 has swapkey=hello, db1 is empty.
+# Use explicit `-n <db>` per invocation — `redis-cli SELECT` does NOT persist
+# across separate process invocations, so the previous `both SELECT 1; both
+# DEL swapkey` deleted from db0 (the just-seeded key) instead of db1.
+redis-cli -p "$PORT_REDIS" -n 0 SET swapkey hello >/dev/null
+redis-cli -p "$PORT_RUST"  -n 0 SET swapkey hello >/dev/null
+redis-cli -p "$PORT_REDIS" -n 1 DEL swapkey >/dev/null
+redis-cli -p "$PORT_RUST"  -n 1 DEL swapkey >/dev/null
 
 # SWAPDB 0 1 — swaps databases 0 and 1
 assert_both "SWAPDB 0 1" SWAPDB 0 1
@@ -566,13 +569,19 @@ assert_eq "SWAPDB: key absent from db0" "$redis_db0_gone" "$rust_db0_gone"
 # Same-index SWAPDB is a no-op; must return OK (not error)
 assert_both "SWAPDB 0 0 (same-index no-op)" SWAPDB 0 0
 
-# Out-of-range indices must return ERR (not panic)
+# Out-of-range indices must return ERR (not panic) — assert parity with Redis,
+# not just that moon emits *some* ERR. The previous check ignored $redis_oor,
+# so a divergence (e.g. moon ERR + Redis OK, or different error wording class)
+# would silently pass.
 redis_oor=$(redis-cli -p "$PORT_REDIS" SWAPDB 0 9999 2>&1) || true
 rust_oor=$(redis-cli -p "$PORT_RUST" SWAPDB 0 9999 2>&1) || true
-if echo "$rust_oor" | grep -qi "ERR"; then
+if echo "$redis_oor" | grep -qi "ERR" && echo "$rust_oor" | grep -qi "ERR"; then
     PASS=$((PASS + 1))
 else
-    FAIL=$((FAIL + 1)); echo "  FAIL: SWAPDB out-of-range should return ERR, got: $rust_oor"
+    FAIL=$((FAIL + 1))
+    echo "  FAIL: SWAPDB out-of-range parity"
+    echo "    redis: $redis_oor"
+    echo "    rust:  $rust_oor"
 fi
 
 # Swap back to restore state for remaining tests

src/cluster/command.rs

@@ -100,20 +100,25 @@ pub fn handle_cluster_myid(cs: &Arc<RwLock<ClusterState>>) -> Frame {
 /// No trailing newline — callers add `\n` for CLUSTER NODES (bulk-string concat) or
 /// wrap in `Frame::BulkString` directly for CLUSTER REPLICAS (array of strings).
 fn format_node_line(node: &ClusterNode, self_node_id: &str) -> String {
+    // CLUSTER NODES flags field is a comma-separated keyword list — never
+    // includes the master-id (that goes in its own column below). Including
+    // master_id here previously produced a malformed line with master_id
+    // appearing twice (once inside flags, once in the dedicated column),
+    // which broke whitespace-tokenized parsers (e.g. redis-cli's own line
+    // splitter). Reference: Redis CLUSTER NODES format spec.
     let flags_str = if node.node_id == self_node_id {
-        // self: prepend "myself,"
         match &node.flags {
-            NodeFlags::Master => "myself,master".to_string(),
-            NodeFlags::Replica { master_id } => format!("myself,slave {}", master_id),
-            NodeFlags::Pfail => "myself,pfail".to_string(),
-            NodeFlags::Fail => "myself,fail".to_string(),
+            NodeFlags::Master => "myself,master",
+            NodeFlags::Replica { .. } => "myself,slave",
+            NodeFlags::Pfail => "myself,pfail",
+            NodeFlags::Fail => "myself,fail",
         }
     } else {
         match &node.flags {
-            NodeFlags::Master => "master".to_string(),
-            NodeFlags::Replica { master_id } => format!("slave {}", master_id),
-            NodeFlags::Pfail => "pfail".to_string(),
-            NodeFlags::Fail => "fail".to_string(),
+            NodeFlags::Master => "master",
+            NodeFlags::Replica { .. } => "slave",
+            NodeFlags::Pfail => "pfail",
+            NodeFlags::Fail => "fail",
         }
     };
 
@@ -922,35 +927,85 @@ mod tests {
         }
     }
 
-    /// T2.4-b: Array with one BulkString per replica, each containing ≥9 fields.
+    /// T2.4-b: Array with one BulkString per replica, each containing exactly
+    /// 9 fields and the correct replica-row layout.
+    ///
+    /// Pinned columns (whitespace-tokenized):
+    ///   [0] node-id            (40 hex chars)
+    ///   [1] ip:port@bus
+    ///   [2] flags              (== "slave" for non-self replica)
+    ///   [3] master-id          (40 hex chars, the master being replicated)
+    ///   [4] ping-sent
+    ///   [5] pong-recv
+    ///   [6] epoch
+    ///   [7] link-state
+    ///   [8] slot-ranges        ("-" for replicas, never empty)
+    ///
+    /// This guards against the previous bug where `format_node_line` embedded
+    /// the master-id inside the flags field, producing "slave <master_id>" and
+    /// shifting every subsequent column right by one.
     #[test]
     fn cluster_replicas_lists_replicas() {
         let (cs, master_id, replica1_id, replica2_id) = make_cs_with_replicas();
-        let args = vec![Frame::BulkString(bytes::Bytes::from(master_id))];
+        let args = vec![Frame::BulkString(bytes::Bytes::from(master_id.clone()))];
         let result = handle_cluster_replicas(&args, &cs);
         match result {
             Frame::Array(items) => {
                 assert_eq!(items.len(), 2, "expected 2 replicas");
-                // Gather node-ids from the returned lines
                 let mut seen_ids = std::collections::HashSet::new();
                 for item in &*items {
                     let line = match item {
                         Frame::BulkString(b) => String::from_utf8(b.to_vec()).unwrap(),
                         other => panic!("expected BulkString element, got {:?}", other),
                     };
-                    // Must not have trailing newline
                     assert!(
                         !line.ends_with('\n'),
                         "line must not end with newline: {:?}",
                         line
                     );
                     let fields: Vec<&str> = line.split_whitespace().collect();
-                    assert!(
-                        fields.len() >= 9,
-                        "expected >= 9 fields, got {}: {}",
+                    // Exactly the pinned columns above; slot-ranges renders
+                    // as "-" for replicas (no slot ownership), so we expect
+                    // exactly 9 whitespace-separated tokens. If
+                    // `format_node_line` ever re-embeds master_id inside the
+                    // flags column, this would become 10 — which is the
+                    // signal we want to lock down.
+                    assert_eq!(
                         fields.len(),
+                        9,
+                        "wrong column count (would silently regress if flags \
+                         re-embed master_id): {:?}",
+                        line
+                    );
+                    // flags column must be exactly "slave" — NOT "slave <hex40>".
+                    assert_eq!(
+                        fields[2], "slave",
+                        "flags column must be 'slave', got '{}': {:?}",
+                        fields[2], line
+                    );
+                    // master-id column must be the 40-char master id of the
+                    // node being replicated, NOT empty and NOT a copy of any
+                    // other field.
+                    assert_eq!(
+                        fields[3].len(),
+                        40,
+                        "master-id column must be 40 hex chars: {:?}",
+                        line
+                    );
+                    assert_eq!(
+                        fields[3], master_id,
+                        "master-id column must match the master being queried: {:?}",
                         line
                     );
+                    // master-id must appear exactly once on the line (the bug
+                    // duplicated it: once in flags, once in its own column).
+                    let occurrences = line.matches(master_id.as_str()).count();
+                    assert_eq!(
+                        occurrences, 1,
+                        "master-id must appear exactly once on the line, found \
+                         {} occurrences: {:?}",
+                        occurrences, line
+                    );
                     seen_ids.insert(fields[0].to_string());
                 }
                 assert!(seen_ids.contains(&replica1_id), "replica1 not in result");

src/command/key_extra.rs

@@ -39,14 +39,34 @@ pub fn copy(db: &mut Database, args: &[Frame]) -> Frame {
         if arg.eq_ignore_ascii_case(b"REPLACE") {
             replace = true;
         } else if arg.eq_ignore_ascii_case(b"DB") {
-            // Cross-db COPY is intercepted by handler-level code before dispatch().
-            // When this branch is reached, the DB index has already been validated
-            // as the same database (parse_copy_db_args returned None → same-db path).
-            // Consume the DB index argument and continue as a same-db copy.
-            i += 1; // skip the db-index token
+            // Cross-db COPY is intercepted by handler-level code before
+            // dispatch(). When this branch is reached, the DB index has
+            // already been validated as the same database (parse_copy_db_args
+            // returned None → same-db path). Even so, defensively validate
+            // the consumed token so a malformed value like `COPY src dst DB
+            // abc` is rejected rather than silently accepted in any path
+            // that bypasses the handler-level intercept (e.g. future
+            // callers, dispatch paths that don't go through parse_copy_db_args).
+            i += 1;
             if i >= args.len() {
                 return Frame::Error(Bytes::from_static(b"ERR syntax error"));
             }
+            let db_tok = match extract_key(&args[i]) {
+                Some(k) => k,
+                None => {
+                    return Frame::Error(Bytes::from_static(
+                        b"ERR value is not an integer or out of range",
+                    ));
+                }
+            };
+            let parsed: Option<usize> = std::str::from_utf8(db_tok)
+                .ok()
+                .and_then(|s| s.parse::<usize>().ok());
+            if parsed.is_none() {
+                return Frame::Error(Bytes::from_static(
+                    b"ERR value is not an integer or out of range",
+                ));
+            }
         } else {
             return Frame::Error(Bytes::from_static(b"ERR syntax error"));
         }