CfdpManager: received files go through tmp_dir; kept and failed files keep their names (#5898)

[binh-lgtm]

Related Issue(s)	#5898
Has Unit Tests (y/n)	y
Documentation Included (y/n)	y
Generative AI was used in this contribution (y/n)	y

---

Change Description

Before: CfdpManager writes a received file straight to its destination from the first PDU, keeps it when the receive fails, and deletes failed poll files instead of moving them.
After: a received file appears at its destination only once its checksum passes; a failed receive leaves nothing behind and an older file of the same name stays; kept and failed files land in move_dir/fail_dir under their own names.

• Every receive starts with keep=DELETE; the success paths already set KEEP (part 3 of Svc/Ccsds/CfdpManager: queue telemetry underflow, move_dir passed as a file path, RX failures never delete the partial file #5898).
• handleNotKeepFile moves into <dir>/<base name> instead of passing the directory itself to rename(), which failed and fell back to deleting (part 2 of Svc/Ccsds/CfdpManager: queue telemetry underflow, move_dir passed as a file path, RX failures never delete the partial file #5898).
• A receive recycled with its file still open, such as a Class 1 receive that times out for inactivity, removes that file instead of only closing it.
• Every receive writes <tmp_dir>/<src>:<seq>.tmp, as the SDD already describes, and is renamed onto its destination before Finished is sent. A failed rename ends the transaction FILESTORE_REJECTION. A late Metadata no longer moves the file, so RxFileReopenFailed is no longer emitted; it stays declared so event ids do not shift. A destination whose directory is missing is still rejected when Metadata arrives.

Rationale

A partial file at the destination can be picked up by whatever consumes that directory, and a failed re-upload destroyed the good copy it was replacing. Failed poll files were lost instead of kept for a retry.

Testing/Review Recommendations

• fprime-util check in Svc/Ccsds/CfdpManager: 199/199.
• New UTs: Class1RxWritesIntoTmpUntilComplete, Class1RxFailureKeepsTheOldFile, Class1RxCrcMismatchRemovesFile, R2LateMetadataToAMissingDirectoryIsRejected, Class1RxInactivityRemovesTheTempFile, MoveDirKeepsTheSentFileByName, FailDirKeepsAFailedPollFileByName; RxFileRenameFailed now covers the rename at completion.
• Also flown in our deployment's SIL (real binary, Class 1 and 2 both ways through frame loss).
• Review focus: Transaction::rCommitFile and its two call sites (R1 at EOF, R2 after the checksum), r2RecvMd, and the cleanup in Channel::recycleTransaction.

Future Work

Part 1 of #5898 (queue telemetry underflow) is not addressed here.

Contributor Checklist

• I have the rights to contribute this change set under the Apache-2.0 license.
• I agree to be bound by the contributor guidelines.

AI Usage (see policy)

Drafted with Claude (code, tests, description); reviewed and tested by the author.

[lestarch-autobot]

[Documentation Currency] must fix r2RecvMd() Doxygen (Transaction.hpp line 716) still says a late Metadata PDU "will move the file to the correct destination according to the metadata PDU"; this PR removes that move.

(Anchored below the offending line; the diff does not include line 716.)
With this change r2RecvMd() only validates the destination directory (rDestinationDirExists(), RxFileCreateFailed / FILESTORE_REJECTION on failure) and the received data stays at m_rxTmpFilename until rCommitFile() renames it into place after the checksum passes. The brief now describes removed behavior beside its replacement. Suggested wording: "If metadata was missed, the file keeps accumulating at its temporary path; this function only checks that the destination directory exists, and rCommitFile() renames the file into place once the transfer completes."

[lestarch-autobot]

[Documentation Currency] could fix RxFileReopenFailed is documented here as "Not emitted", but Events.fppi still declares it as a live event ("RX transaction failed to reopen file {} after rename") with no indication it is dead.

Ground dictionaries generated from the model will keep advertising an event no code path raises. Either annotate the FPP event as retained for dictionary compatibility / not emitted, or remove the event and this row together. Tagged could fix rather than must fix because this SDD row already tells operators the truth; only the FPP annotation is stale.

[lestarch-autobot]

[Documentation Currency] could fix The SDD describes the temporary-file happy path (Assumption 7; ChannelConfig.tmp_dir row near line 621) but not the failure semantics this PR introduces.

(Anchored on the nearest diff line; the affected rows are outside the hunk.)
Worth stating beside the tmp_dir row: a failed or cancelled receive (CRC mismatch, inactivity, rename failure, channel recycle) removes the temporary file and leaves any existing destination file untouched; a rename failure ends the transaction with FILESTORE_REJECTION plus RxFileRenameFailed. Tagged could fix because the current wording ("moved to their final destination upon successful completion") is incomplete rather than false.

cc @LeStarch @thomas-bc — low-confidence finding, please confirm.

[lestarch-autobot]

[C++ Design] must fix CPP-36 (event not uniquely traceable): RxFileCreateFailed is now emitted from two sites that can produce an identical argument set.

rInit() (R2 with metadata first, destination directory missing) and this late-metadata branch both emit RxFileCreateFailed(CLASS_2, src_eid, seq_num, dst_filename, DOESNT_EXIST), so an operator reading the downlinked event cannot tell which path rejected the transfer. Before this PR the event had a single emission site in rInit(). Either fold the directory check + rejection into one helper shared by both paths (single log_* call), or give this site its own event (e.g. RxDestinationDirMissing).

[lestarch-autobot]

[C++ Design] could fix CPP-32 (ignored return value): the Fw::FormatStatus returned by this path-building format() is discarded, so a truncated m_rxTmpFilename would be created, written and later moved as if it were the intended temp path.

Engine::moveIntoDir() in this same PR checks the status and returns OTHER_ERROR; do the same here and treat a non-SUCCESS status like the open failure below (RxFileCreateFailed + FILESTORE_REJECTION). Tagged below must-fix because with the shipped sizes (MaxFilePathSize 200 + / + two 20-digit ids + : + .tmp = 246 < FW_FIXED_LENGTH_STRING_SIZE 256) truncation is unreachable; it only becomes reachable if a project raises MaxFilePathSize, at which point two transactions can collide on one truncated temp name.

[lestarch-autobot]

[Security] Concur — also in scope for ground-input validation: tmp_dir (ground-settable ChannelConfig param, ≤ MaxFilePathSize) plus uplinked src_eid/seq_num build this path, and a truncated OVERFLOWED name is shared by every receive on the channel, so concurrent transactions overwrite, move, or delete each other's data; unreachable with default sizes (200 + two U32 ids = 226 < 256), hence could fix.

[lestarch-autobot]

[C++ Design] could fix CPP-32 (ignored return value): the (void) discard of removeFile() carries no justification, and a temp file that survives this cleanup is not reported anywhere.

Engine::handleNotKeepFile() emits FileRemoveFailed for the equivalent cleanup; here the rename failure is already reported one line above, so at minimum say why the remove status is dropped.

Suggested change

	(void)Os::FileSystem::removeFile(this->m_rxTmpFilename.toChar());
	(void)Os::FileSystem::removeFile(this->m_rxTmpFilename.toChar()); // best effort; the rename failure is already reported above

[lestarch-autobot]

[C++ Design] could fix CPP-32 (ignored return value): (void) discard without a justifying comment; a temp file that survives this cleanup is never reported.

DanglingFileHandleClosed is already emitted on this path, so a one-line justification is enough if silent best-effort removal is intended; otherwise report the failure the way Engine::handleNotKeepFile() does.

Suggested change

	(void)Os::FileSystem::removeFile(txn->m_rxTmpFilename.toChar());
	(void)Os::FileSystem::removeFile(txn->m_rxTmpFilename.toChar()); // best effort; DanglingFileHandleClosed already reported this path

[lestarch-autobot]

[Summary] Severity carried from duplicate — the consolidated Maintainability thread #6118 (comment) tagged this same unreported removeFile failure suggestion (Design concurred there); the summary counts this finding as suggestion. The original tag above stands as posted.

[lestarch-autobot]

[C++ Design] could fix CPP-33 (inlined utility): path splitting (strrchr('/') then %.*s / slash + 1) is now hand-rolled in three places in this component: Engine::isPollingDir() (pre-existing), this helper, and Transaction::rDestinationDirExists().

The logic depends on no component state and is useful beyond CFDP; factor one dirname/basename helper (e.g. in CfdpManager/Utils.hpp, or alongside Os::FileSystem) and call it from all three sites so the edge cases (no slash, leading /, trailing /) are handled once.

[lestarch-autobot]

[Maintainability] could fix rCommitFile() returns a bare bool where the rest of this file's R-helpers return Cfdp::Status::T (rCheckCrc, r2CalcCrcChunk, rSubstateSendNak) and its body already holds the Os::FileSystem::Status it discards. Because the existing helpers are tested with mixed truthiness in the same file (if (this->r2CalcCrcChunk()) means failure at line ~1046, while the new if (this->rCommitFile()) means success), the polarity of a bare-truthiness call is not locally obvious here.

Maintenance cost: the next engineer touching the R1/R2 completion branches has to open the callee to learn which way true goes, and a slip inverts whether a file is retained. Returning Os::FileSystem::Status (or Cfdp::Status::T) and comparing against OP_OK/SUCCESS at the two call sites makes the branch self-describing; the change touches Transaction.hpp and both callers, so no single-hunk suggestion is offered.

[lestarch-autobot]

[Maintainability] could fix This missing-destination-directory rejection (log RxFileCreateFailed with DOESNT_EXIST, incrementFaultFileOpen, r2SetFinTxnStatus(FILESTORE_REJECTION)) is the second copy of the one added to rInit() (lines ~362-375). The rInit() copy reaches the same event by synthesizing an Os::File::Status into the variable that otherwise carries the real open() result and swapping a failedFile pointer, so a reader of either site has to reconstruct that "directory missing" and "open failed" are reported as the same thing on purpose.

Maintenance cost: any later change to how a missing destination directory is reported (different event, different fault counter, a retry on open) has to be mirrored in two places that look different. A small rRejectMissingDestinationDir() (or having rDestinationDirExists() do the reporting) would give one site; since it spans two functions and the header, no single-hunk suggestion is offered.

[lestarch-autobot]

[Maintainability] could fix This "directory part of a path" split is nearly identical to the one in Engine::isPollingDir() (Engine.cpp lines ~1101-1113: manual last-/ scan, then the same %.*s format), and the new Engine::moveIntoDir() adds a third strrchr-based split for the basename. The two directory splits already disagree at the edges: this one special-cases a root-level path (/file -> /), isPollingDir() yields an empty directory for the same input.

Maintenance cost: a fix to path handling (trailing slash, root, relative path) must now be applied in three places in two files using two idioms, and the existing divergence shows that already does not happen. A shared splitPath(const Fw::StringBase&, Fw::String& dir, const char*& name) next to the other helpers in Utils.hpp would give one place to get this right; the change spans files, so no single-hunk suggestion is offered.

[lestarch-autobot]

[Maintainability] could fix setDirectories() does more than its name says: it rebuilds the entire ChannelConfig parameter for the channel (ack/nack limits, both timers, dequeue enable, tmp_dir, PDU-per-cycle) with fixed values and sends it, so any test that configures other channel params and then calls it has them silently overwritten. It is also declared in the test-case list (PDU Processing and Serialization Failure Events) rather than with the other harness helpers under Helper functions, where the next person looking for "how do I configure a channel in a test" will search.

Maintenance cost: a hidden full-parameter reset is the kind of thing that produces a puzzling test failure one refactor later. A name that states the effect (e.g. configureChannelWithDirs(...), or passing a Cfdp::ChannelParams& to mutate instead of three positional const char*) and moving the declaration under Helper functions would make both visible; the rename touches the two call sites, so no single-hunk suggestion is offered.

[lestarch-autobot]

[Correctness] must fix A Class 1 receive whose EOF checksum does not match now discards the received file but still reports the transfer as completed.

Finding class: correctness-ignored-status

Trigger: Class 1 receive; Metadata, all FileData and an EOF PDU with condition code NO_ERROR and a checksum that does not match the received data (exactly the sequence testClass1RxCrcMismatchRemovesFile sends).

What happens: rCheckCrc() returns ERROR and emits RxCrcMismatch, but no transaction status is recorded for the failure (only the rename-failure branch sets FILESTORE_REJECTION). r1Reset() then calls Engine::finishTransaction(), which — because this PR now starts every RX transaction with m_keep = Keep::DELETE — closes the handle and removes m_rxTmpFilename via handleNotKeepFile(), and then, since txn_stat is still TXN_STATUS_UNDEFINED (TxnStatusIsError() false), emits RxFileTransferCompleted(src -> dst_filename) instead of RxFileTransferFailed.

Consequence: the ground receives a success event naming a destination file that does not exist (and, with the old file preserved per this PR's intent, that may still hold stale content). Before this PR the same path kept the mismatched file at dst_filename, so the completion event at least pointed at an existing file; the PR makes the event false. The missing status is pre-existing, but the DELETE default this PR adds is what turns it into a false success report, so it is tagged on consequence (contract §1a). The Class 1 size-mismatch path in rSubstateRecvEof() (REC_PDU_FSIZE_MISMATCH_ERROR, no status set) has the same shape. The new test testClass1RxCrcMismatchRemovesFile does not assert on the completion/failure events, so it does not catch this.

Suggested change

	if (this->rCheckCrc(crc) == Cfdp::Status::SUCCESS) {
	/* successfully processed the file */
	this->m_keep = Cfdp::Keep::KEEP; /* save the file */
	if (this->rCommitFile()) {
	this->m_keep = Cfdp::Keep::KEEP; /* save the file */
	} else {
	this->m_engine->setTxnStatus(this, TxnStatus::TXN_STATUS_FILESTORE_REJECTION);
	}
	}
	if (this->rCheckCrc(crc) == Cfdp::Status::SUCCESS) {
	/* successfully processed the file */
	if (this->rCommitFile()) {
	this->m_keep = Cfdp::Keep::KEEP; /* save the file */
	} else {
	this->m_engine->setTxnStatus(this, TxnStatus::TXN_STATUS_FILESTORE_REJECTION);
	}
	} else {
	this->m_engine->setTxnStatus(this, TxnStatus::TXN_STATUS_FILE_CHECKSUM_FAILURE);
	}

[lestarch-autobot]

[Operational] Concur — also in scope for failure-path consequences: the operator sees RxFileTransferCompleted naming a destination that holds no new file (or stale content), so an uplink that must be re-sent looks finished on the ground.

[lestarch-autobot]

[Correctness] could fix If the temp file cannot be reopened for the Class 2 CRC pass, it is never removed.

Finding class: correctness-resource-leak

Trigger: Class 2 receive, all data received, r2CalcCrcChunk() closes the write handle and m_fd.open(m_rxTmpFilename, OPEN_READ) fails (filesystem fault; the file was just written and closed).

What happens: the status becomes FILE_SIZE_ERROR and the FIN is sent with DISCARDED, but m_fd is now closed. Every later cleanup is gated on an open handle — Engine::finishTransaction() only calls handleNotKeepFile() inside if (m_fd.isOpen()), and Channel::recycleTransaction() only removes the temp file inside the same guard — so m_rxTmpFilename stays in tmp_dir after the transaction is freed.

Consequence: an orphaned temp file in tmp_dir (until a later transaction with the same source EID and sequence number overwrites it), while the FIN told the sender the file was discarded. Below must-fix because it needs an OS-level open failure on a file that was just closed; before this PR the same fault left the partial file at the destination instead.

Suggested change

	this->m_engine->setTxnStatus(this, TxnStatus::TXN_STATUS_FILE_SIZE_ERROR);
	this->m_engine->setTxnStatus(this, TxnStatus::TXN_STATUS_FILE_SIZE_ERROR);
	(void)Os::FileSystem::removeFile(this->m_rxTmpFilename.toChar());

[lestarch-autobot]

[Operational] Concur — also in scope for failure-path consequences: up to m_fsize bytes stay in tmp_dir for the mission with no sweep; removing the temp from finishTransaction() whenever m_rxTmpFilename is set would also cover future closed-handle exits.

[lestarch-autobot]

[Test Quality] must fix The late-metadata path this PR rewrote is only tested on its reject branch; the accept branch (directory exists → keep writing the temp file → commit into the late-supplied dst_filename at EOF) has no test.

This test calls r2RecvMd directly with a bad directory. No test in the suite sends FileData before Metadata and then drives Metadata + EOF through the component to verify the file lands at the Metadata destination and the temp file is gone (the existing testClass2Rx* helpers all send Metadata first; testRxTempFileCreatedEvent stops after the first FileData). The removed testRxFileReopenFailedEvent was the only coverage of this branch, so the PR's main Class 2 behaviour change is unverified. A component-driven test modelled on testClass1RxWritesIntoTmpUntilComplete (Class 2, FileData → Metadata → EOF, then run1Hz loop, verifyReceivedFile(dst), NOT_EXIST on the temp path) closes the gap.

[lestarch-autobot]

[Test Quality] suggestion Count-only assertion on RxInactivityTimeout; the class/srcEid/seqNum payload is deterministic here and not checked.

This path also emits DanglingFileHandleClosed from Channel::recycleTransaction (the file is still open when the inactivity timer fires), which the test leaves unconstrained — assert it explicitly so the test documents whether that warning is expected on every Class 1 inactivity.

Suggested change

	ASSERT_EVENTS_RxInactivityTimeout_SIZE(1);
	ASSERT_EVENTS_RxInactivityTimeout_SIZE(1);
	ASSERT_EVENTS_RxInactivityTimeout(0, Cfdp::Class::CLASS_1, sourceEid, transactionSeq);
	ASSERT_EVENTS_DanglingFileHandleClosed_SIZE(1);

[lestarch-autobot]

[Test Quality] suggestion Count-only assertion on RxCrcMismatch; the deterministic payload (class, srcEid, seqNum, expected = the EOF checksum) is not checked, and no other test asserts this event for Class 1.

actual is the computed CRC, so assert the fields individually as testRxCrcMismatchEvent does.

Suggested change

	ASSERT_EVENTS_RxCrcMismatch_SIZE(1);
	ASSERT_EVENTS_RxCrcMismatch_SIZE(1);
	ASSERT_EQ(Cfdp::Class::CLASS_1, this->eventHistory_RxCrcMismatch->at(0).cfdpClass);
	ASSERT_EQ(sourceEid, this->eventHistory_RxCrcMismatch->at(0).srcEid);
	ASSERT_EQ(transactionSeq, this->eventHistory_RxCrcMismatch->at(0).seqNum);
	ASSERT_EQ(wrongChecksum, this->eventHistory_RxCrcMismatch->at(0).expected);

[lestarch-autobot]

[Test Quality] suggestion Count-only assertion on RxCrcMismatch; the deterministic payload (class, srcEid, seqNum, expected = the EOF checksum) is not checked, and no other test asserts this event for Class 1.

actual is the computed CRC, so assert the fields individually as testRxCrcMismatchEvent does.

Suggested change

	ASSERT_EVENTS_RxCrcMismatch_SIZE(1);
	ASSERT_EVENTS_RxCrcMismatch_SIZE(1);
	ASSERT_EQ(Cfdp::Class::CLASS_1, this->eventHistory_RxCrcMismatch->at(0).cfdpClass);
	ASSERT_EQ(sourceEid, this->eventHistory_RxCrcMismatch->at(0).srcEid);
	ASSERT_EQ(transactionSeq, this->eventHistory_RxCrcMismatch->at(0).seqNum);
	ASSERT_EQ(0xDEADBEEFu, this->eventHistory_RxCrcMismatch->at(0).expected);

[lestarch-autobot]

[Test Quality] could fix The Class 1 receive driver (tmp-path format, sendMetadataPdu → doDispatch → sendFileDataPdu → doDispatch → sendEofPdu → doDispatch with the same 10-byte payload) is repeated verbatim in five tests in this hunk set (CrcMismatchRemovesFile, WritesIntoTmpUntilComplete, FailureKeepsTheOldFile, InactivityRemovesTheTempFile, RxFileRenameFailedEvent).

A helper taking (seq, dst, eofCrc, sendEof) returning the temp path would leave each test as setup + assertions and make the per-test intent visible.

[lestarch-autobot]

[Test Quality] could fix The Class 2 commit-failure branch (rCommitFile() false after a good CRC → FIN FILESTORE_REJECTION instead of RETAINED/COMPLETE) has no test; testRxFileRenameFailedEvent only covers the Class 1 branch.

This is the branch that decides what the sender is told about the file, so a Class 2 variant of that test (destination is an existing directory, run run1Hz until the FIN is sent, assert RxFileRenameFailed_SIZE(1) and the FIN file-status) would guard it.

[lestarch-autobot]

[Design] must fix rCommitFile() relies on Os::FileSystem::moveFile, whose cross-device fallback re-creates the very failure this PR exists to remove.

moveFile is rename() plus, on EXDEV_ERROR, copyFile + removeFile (Os/FileSystem.cpp:209-225). copyFile opens the destination with OPEN_CREATE/OVERWRITE (O_TRUNC) before copying and does not clean up on failure, so when tmp_dir (default "/tmp", on Linux usually a separate tmpfs) is not on the destination's filesystem: (a) the existing good destination file is truncated before the new data lands, (b) a copy that fails midway (ENOSPC is the realistic case) leaves a partial file at the destination while this function removes only the temp file, and (c) the whole-file copy runs synchronously inside the EOF handler / tick, defeating the per-cycle bounding that r2CalcCrcChunk goes to lengths to keep. The PR description promises "a failed receive leaves nothing behind and an older file of the same name stays"; with the default parameter that holds only when tmp_dir happens to share a filesystem with the destination, which the design nowhere requires. Either make the commit atomic-or-rejected (rename only, below, and state in the SDD tmp_dir row that it must be on the destination filesystem) or stage the copy as a sibling temp name in the destination directory and rename that into place.

Suggested change

	Os::FileSystem::Status status =
	Os::FileSystem::moveFile(this->m_rxTmpFilename.toChar(), this->m_history->fnames.dst_filename.toChar());
	Os::FileSystem::Status status =
	Os::FileSystem::rename(this->m_rxTmpFilename.toChar(), this->m_history->fnames.dst_filename.toChar());

[lestarch-autobot]

[Design] suggestion A Class 1 receive whose destination directory is missing (or whose temp file cannot be created) ends with RxFileTransferCompleted.

(Anchored on the R2 branch; the offending r1Reset() two lines below is outside the hunk.) The R1 branch logs RxFileCreateFailed and calls r1Reset() without recording an error status; finishTransaction then sees a non-error txn_stat and emits the RX completion event for a transfer that never had a file. The R2 branch correctly sets FILESTORE_REJECTION; setting it for both classes before the R2-only FIN flag reports the failure as a failure. The structure predates this PR, which is why this is a suggestion rather than must fix, but the PR adds the rDestinationDirExists() rejection to this exact branch and new UTs around it, so it is the right moment to close it (same false-success shape as the Correctness thread on the Class 1 CRC-mismatch path).

Suggested change

	if (this->m_state == TxnState::TXN_STATE_R2) {
	this->r2SetFinTxnStatus(TxnStatus::TXN_STATUS_FILESTORE_REJECTION);
	this->m_engine->setTxnStatus(this, TxnStatus::TXN_STATUS_FILESTORE_REJECTION);
	if (this->m_state == TxnState::TXN_STATE_R2) {
	this->m_flags.rx.send_fin = true;

[lestarch-autobot]

[Summary] Promoted to must fix — the rationale states a Class 1 receive whose destination directory is missing (or whose temp file cannot be created) logs RxFileCreateFailed and then ends with RxFileTransferCompleted: success reported for an operation that was rejected.

The summary counts this finding as must fix; the original tag above stands as posted.

[lestarch-autobot]

[Operational] could fix dst is an Fw::String (FW_FIXED_LENGTH_STRING_SIZE = 256), but dir and the source basename are each legal up to MaxFilePathSize = 200, so dir + "/" + name up to 401 chars overflows and the move is reported as OTHER_ERROR.

ops-config-extreme. handleNotKeepFile() then falls through to removeFile, so with e.g. a 150-char fail_dir and a 110-char poll filename the failed poll file is deleted instead of preserved, with only FailPollFileMove(status=OTHER_ERROR) to tell the operator why the forensic copy is gone. Not a regression (the old moveFile(src, dir) always failed), but the new fallback is silent about the cause. Smallest remedy: format into an Fw::ExternalString over a char[2 * MaxFilePathSize + 2] buffer, or document the combined length limit on the SDD fail_dir/move_dir rows.

[lestarch-autobot]

Automated review summary (run 1)

Per-agent results

Agent	must fix	suggestion	could fix	future work	outstanding	Verdict
Security Vulnerabilities	0	0	1	0	1	Go
Supply Chain / Runner Safety	0	0	0	0	0	Go
F Prime C/C++ Design	1	0	4	0	5	No-Go
Documentation Currency	1	0	2	0	3	No-Go
Design	2¹	0	1	0	3	No-Go
Architecture	0	0	0	0	0	Go
Test Quality	1	3	2	0	6	No-Go
Correctness	1	0	1	0	2	No-Go
Operational	2	0	2	0	4	No-Go
Maintainability	0	1	4	0	5	Go
CI safety	—	—	—	—	—	Go
Totals²	6	4	13	0	23	No-Go

¹ Includes one suggestion promoted to must fix by severity reconciliation (see below).
² Totals count each open thread once: 4 cross-agent concurrences and the 2 duplicates closed this run are folded into their canonical threads at the stricter tag.

Duplicates consolidated this run: 2 (threads closed by the §5h post-pass)

Supply-chain surfaces

Surface	Outstanding
Dependencies	clean
Vendored / submodule	clean
Build / test infrastructure	clean
Workflows / actions / scripts	clean
Generator output	clean
Prompt-injection	clean
Review-system integrity	clean

Outstanding must-fix items (6)

F Prime C/C++ Design

• CPP-36: RxFileCreateFailed now emitted from two sites with an identical argument set (rInit() and the late-metadata branch), so the event is no longer uniquely traceable — #6118 (comment)

Documentation Currency

• r2RecvMd() Doxygen still says a late Metadata PDU moves the file to its destination; this PR removes that move — #6118 (comment)

Design

• Human design adjudication required. rCommitFile() relies on Os::FileSystem::moveFile, whose cross-device (EXDEV) fallback truncates the existing destination, can leave a partial file on a failed copy, and runs a synchronous whole-file copy on the CfdpManager thread with the default tmp_dir = "/tmp" — also: Operational — #6118 (comment)
• Human design adjudication required. Class 1 receive with a missing destination directory (or temp-file create failure) logs RxFileCreateFailed yet ends with RxFileTransferCompleted (promoted from suggestion, see Severity reconciliation) — #6118 (comment)

Test Quality

• The rewritten late-metadata path is tested only on its reject branch; the accept branch (FileData → Metadata → EOF, file lands at the Metadata destination, temp file gone) has no test — #6118 (comment)

Correctness

• Class 1 receive with an EOF checksum mismatch now discards the file but still emits RxFileTransferCompleted naming a destination that holds no new file — also: Operational — #6118 (comment)

Severity reconciliation (1 promoted)

Finding	Reviewer tag	Summary tag	Consequence	Link
Class 1 missing destination dir ends with RxFileTransferCompleted	suggestion	must fix	success event reported for a rejected transfer (RxFileCreateFailed then RxFileTransferCompleted); tagged in-scope by the reviewer	#6118 (comment)
RxFileReopenFailed SDD row says "Not emitted" while Events.fppi still describes it as live	could fix	could fix (not promoted)	rationale states the SDD row is true; only the FPP description is stale (not an sdd/manual/Doxygen claim made false)	#6118 (comment)
SDD omits failure semantics of the temp-file receive path	could fix	could fix (not promoted)	rationale states the SDD wording is incomplete, not false	#6118 (comment)
Discarded format() status for m_rxTmpFilename (Security concurs)	could fix	could fix (not promoted)	truncation stated unreachable with shipped sizes; reachable only if a project raises MaxFilePathSize	#6118 (comment)
(void) removeFile() in rCommitFile() unjustified	could fix	could fix (not promoted)	no §14 consequence asserted (reporting/justification gap)	#6118 (comment)
(void) removeFile() in Channel::recycleTransaction unreported (canonical; Maintainability duplicate tagged suggestion, Design concurs)	could fix / suggestion	suggestion (not promoted)	no §14 consequence asserted (unreported best-effort cleanup)	#6118 (comment)
CPP-33 path splitting hand-rolled in three places	could fix	could fix (not promoted)	no §14 consequence asserted	#6118 (comment)
rCommitFile() returns bare bool with non-obvious polarity	could fix	could fix (not promoted)	no §14 consequence asserted	#6118 (comment)
Second copy of the missing-destination-directory rejection	could fix	could fix (not promoted)	no §14 consequence asserted	#6118 (comment)
Directory-split logic duplicated across rDestinationDirExists() / isPollingDir() / moveIntoDir()	could fix	could fix (not promoted)	edge divergence stated as pre-existing; no §14 consequence asserted	#6118 (comment)
setDirectories() silently rebuilds the whole ChannelConfig	could fix	could fix (not promoted)	test-harness naming; no §14 consequence asserted	#6118 (comment)
Temp file never removed when the Class 2 CRC-pass reopen fails (Operational concurs)	could fix	could fix (not promoted)	rationale frames an orphaned temp file after an OS-level open fault as a resource leak; FIN DISCARDED matches the destination outcome, no false success asserted	#6118 (comment)
Count-only assertion on RxInactivityTimeout	suggestion	suggestion (not promoted)	test-assertion strength; no §14 consequence	#6118 (comment)
Count-only assertion on RxCrcMismatch (line 726)	suggestion	suggestion (not promoted)	test-assertion strength; no §14 consequence	#6118 (comment)
Count-only assertion on RxCrcMismatch (line 817)	suggestion	suggestion (not promoted)	test-assertion strength; no §14 consequence	#6118 (comment)
Class 1 receive driver repeated verbatim in five tests	could fix	could fix (not promoted)	no §14 consequence asserted	#6118 (comment)
Class 2 commit-failure FIN branch untested	could fix	could fix (not promoted)	coverage gap; no §14 consequence asserted	#6118 (comment)
dir + "/" + name can exceed Fw::String in moveIntoDir()	could fix	could fix (not promoted)	format status is checked and reported as OTHER_ERROR/FailPollFileMove; no assert, no false success; stated as not a regression	#6118 (comment)

Merge readiness

Merge readiness: No-Go — 6 outstanding must-fix items across C/C++ Design, Documentation Currency, Design (one promoted), Test Quality and Correctness (Operational concurs on two).

Agents that did not run on this PR

• none — all ten registered reviewers completed.

---

Six course corrections before this file transfer clears the launch pad — the receive path is nearly flight-ready.