fix(cli): harden the failure sink (evict after write, hex reference, argv policy)

[Chris0Jeky]

Summary

Takes all three failure-sink follow-ups the #2573 review recorded, each pinned by a regression that
fails against the current sink first.

1. CliFailureSink.TryRecord now evicts old records only after the new record's stream has been
   written and closed, and never evicts the record it just wrote. A create that fails deletes
   nothing.
2. TryRecord validates the correlation reference itself: lowercase hex of exactly 12 characters
   (the generated reference) or 32 (the harness trace correlation). Anything else fails open,
   returning false with nothing written and nothing printed.
3. The argv retention policy is decided and implemented as the conservative option. The record
   keeps the command grammar and drops the values: a token is written verbatim only when it starts
   with - (a flag name) or is one of the at most two leading command words, and every other token
   becomes the fixed placeholder [value]. SensitiveDataRedactor.Redact still runs over the
   result for the attached key=value forms. The policy is written into the sink paragraph of
   docs/security/SECURITY_LOGGING_REDACTION.md.

The #2466 stderr constants, the record's other fields, the 8 KB and 20-record bounds, the
FileMode.CreateNew write and the POSIX 0600 creation mode are unchanged. No file outside
backend/src/Taskdeck.Cli, backend/tests/Taskdeck.Cli.Tests and that one doc paragraph is
touched.

Root cause

Item 1 was an ordering bug. Eviction ran before the write and sized itself for the record about to
be added (existing.Length - MaximumRecordCount + 1), so a create that then failed left the
directory short by up to six records with nothing written in their place: net diagnostic loss
instead of the fail-open-with-no-change the sink documents. Eviction now runs after the stream is
closed, sizes itself against what is actually on disk (existing.Length - MaximumRecordCount) and
skips the just-written path by an ordinal comparison so a record can never evict itself.

Item 2 was an invariant that lived only in the callers. The reference is interpolated into the file
name, and TryRecord checked only for null or whitespace, so a reference that contained path
segments could steer the write out of the diagnostics directory. Both current callers happen to
produce a safe shape; the check now holds regardless of what a future caller does.

Item 3 was a retention question rather than a defect. The redactor only masks the key=value and
key: value forms, so --token abc123 would have been retained verbatim, and ordinary user
content such as a card title reached disk on failure where nothing was retained before the sink
existed.

Verification

All commands run from the worktree at
C:/Users/jekyt/source/Taskdeck-Beta/.worktrees/codex-2577-sink-followups.

Red, before the fix (the tests were committed first, against the unchanged sink):

dotnet test backend/tests/Taskdeck.Cli.Tests/Taskdeck.Cli.Tests.csproj -c Release -m:1 --filter "FullyQualifiedName~CliFailureSinkTests"
-> Failed: 7, Passed: 17, Skipped: 0, Total: 24. The seven failures were:

• TryRecord_WhenTheWriteFailsAtTheCap_DeletesNoOlderRecord -> "Expected remaining to be a
  collection with 20 item(s), but ... contains 1 item(s) less than", the missing entry being
  cli-failure-20260904T112214Z-aaaaaaaaaaaa.txt, deleted by the pre-write eviction for a write
  that then failed on the planted file.
• TryRecord_WithAReferenceThatIsNotLowercaseHex_FailsOpenAndWritesNothing for the references
  a/../../x, 0A1B2C3D4E5F, 0a1b2c3d4e5, 0a1b2c3d4e5f0 and zzzzzzzzzzzz -> "Expected
  captured to be False, but found True". The a/../../x case is the load-bearing one: the old code
  returned true after writing the record outside the diagnostics directory. The ../x, empty and
  whitespace cases already passed, since Windows path normalisation and the old whitespace guard
  happened to reject them; they stay in the theory as regression cover.
• TryRecord_KeepsCommandAndFlagNamesButNoArgumentValues -> the record contained
  argv: cards add --title Secret plan --token abc123 instead of
  argv: cards add --title [value] --token [value].

Green, after the fix:

• dotnet test backend/tests/Taskdeck.Cli.Tests/Taskdeck.Cli.Tests.csproj -c Release -m:1 --filter "FullyQualifiedName~CliFailureSink|FullyQualifiedName~CliUnexpectedError"
  -> Failed: 0, Passed: 37, Skipped: 0, Total: 37.
• dotnet test backend/tests/Taskdeck.Cli.Tests/Taskdeck.Cli.Tests.csproj -c Release -m:1 (whole
  project) -> Failed: 0, Passed: 217, Skipped: 0, Total: 217, duration 3 m 30 s.
• dotnet build backend/Taskdeck.sln -c Release -> 12 Warning(s), 0 Error(s). All twelve
  warnings are pre-existing nullable warnings in Taskdeck.Api.Tests and CliStartupTraceTests,
  none in a file this PR touches.
• node scripts/check-docs-governance.mjs -> "Docs governance check passed."
• git diff --check -> clean.

Not verified

• The full solution test run (dotnet test backend/Taskdeck.sln -c Release -m:1) was not run here;
  ci-required repeats it on this push.
• The POSIX 0600 assertion in Record_IsOwnerReadWriteOnly_OnPosix self-skips on Windows, so it was
  not exercised on this box. The Linux CI leg is what proves the creation mode, and this PR does not
  change it.
• No frontend, E2E or manual CLI run: nothing outside the CLI failure sink changed.
• The new eviction ordering was not exercised against a real full disk or a delete-but-not-create
  directory ACL; the planted-file case is the stand-in for that whole class.

Risk notes

• The reference check accepts lowercase hex only, while CliStartupTrace.IsCorrelationId uses
  Uri.IsHexDigit, which also accepts uppercase. A harness that sets
  TASKDECK_CLI_TEST_TRACE_CORRELATION to an uppercase 32-hex value would now be refused by the
  sink. The consequence is bounded: the startup trace still records the failure, so the run still
  prints its correlation and no "diagnostics were not captured" notice appears; only the sink's
  extra copy is skipped. The in-repo harness uses Guid.NewGuid().ToString("N"), which is
  lowercase.
• The argv policy is deliberately lossy. A record now shows which command and which flags were in
  play but not the values, so a failure that depends on a specific value (a path, an id) is harder
  to reproduce from the record alone. That is the trade the issue asked for.
• A leading command word is retained only when it is at most 32 characters of lowercase letters,
  digits and hyphens, and only in the first two positions and only before the first flag. A
  positional value that happens to have that exact shape and sits in one of those two positions
  would still be retained. Every command group and command the dispatcher routes fits inside the
  two-token depth, so this is the narrowest rule that keeps records readable.
• Eviction now runs after the write, so the directory can briefly hold MaximumRecordCount + 1
  records: the moment between the new record closing and the trim completing. If the process dies in
  that window the next successful write trims it back.

Closes #2577

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[Chris0Jeky]

Fix round

Head 1e1c60090 (809b77194 code + tests, 1e1c60090 doc). All four findings fixed, none declined.

#	Finding	Disposition
1	MEDIUM — eviction inside the write try makes TryRecord return false for a durably written record	fixed
2	MEDIUM — --prefixed token kept whole, so --title=Secret plan reached disk verbatim	fixed
3	LOW — IsAcceptedReference lowercase-only vs CliStartupTrace.IsCorrelationId; duplicated 32	fixed
4	LOW — self-eviction skip branch untested	fixed

1 — eviction can no longer downgrade a written record

EvictOldestRecords now runs in its own try/catch after the stream is closed, so an
enumeration failure leaves the return value alone: the record is durable by then, and the CLI
must not print the diagnostics-unavailable notice for a record that exists. ForDataDirectory
gained an internal record-lister seam (test-only overload) so the test can make the enumeration
throw deterministically on both platforms.

New test TryRecord_WhenEvictionFails_StillReportsTheAlreadyWrittenRecordAsKept — red before the
fix (Expected captured to be True, but found False), green after.

2 — attached values are replaced too

A --prefixed token now keeps only the part up to and including its first =, then appends the
[value] placeholder; Redact still runs over the whole line, so a key the redactor knows still
ends up [redacted].

New test TryRecord_ReplacesAnAttachedValueEvenWhenTheFlagIsNotASecretKeyword for
cards add --title=Secret plan --token=abc123. Red before the fix — the record read
argv: cards add --title=Secret plan --token=[redacted]; now --title=[value] --token=[redacted],
with neither Secret plan nor abc123 present.

Collateral: TryRecord_TruncatesAnOversizedRecordWithAMarker no longer exceeded the 8 KB bound
(7498 bytes) because its fixture put the bulk in attached values, which are now dropped. Its argv
fixture moved the bulk into the flag names, which the policy still retains, so the test exercises
truncation again.

3 — hex in either case, one length constant

IsAcceptedReference now uses Uri.IsHexDigit, the same predicate CliStartupTrace uses, so the
sink cannot refuse a reference the CLI itself printed. CliStartupTrace gained
internal const int CorrelationLength = 32 (used by its own IsCorrelationId), and the sink's
TraceCorrelationLength is now defined as that constant rather than a hand-copied literal.
IsCorrelationId went private -> internal so the test can assert the two agree.

New tests: TryRecord_AcceptsEveryCorrelationTheStartupTraceAccepts (asserts
CliStartupTrace.IsCorrelationId is true and the sink writes the record, for a lowercase guid, an
uppercase guid, and all-a/all-F 32-char references) and
TryRecord_AcceptsAGeneratedReferenceInEitherCase (12-char upper and mixed). The traversal-shaped
refusals stay pinned: ../x, a/../../x, wrong lengths, non-hex, empty and whitespace still fail
open and write nothing — only the 0A1B2C3D4E5F case moved out of that theory, into the accepting
one.

4 — the self-eviction skip branch

New test TryRecord_WhenTheNewRecordIsTheOldestAtTheCap_EvictsAnotherAndKeepsItself: 20 seeded
records all newer than the one being written, so ordinal name order puts the new record first and
only the skip branch can save it. The new record survives, the oldest seeded record goes, the count
returns to the cap. This one passed on first run, as expected for a missing-test finding.

Verification (all run in the PR worktree, at head 1e1c60090)

• dotnet test backend/tests/Taskdeck.Cli.Tests/Taskdeck.Cli.Tests.csproj -c Release -m:1 --filter "FullyQualifiedName~CliFailureSink|FullyQualifiedName~CliUnexpectedError" — 42 passed, 0 failed. (Red run before the fixes: 5 failed, 24 passed.)
• dotnet test backend/tests/Taskdeck.Cli.Tests/Taskdeck.Cli.Tests.csproj -c Release -m:1 — 222 passed, 0 failed, 0 skipped.
• dotnet build backend/Taskdeck.sln -c Release — 0 errors, 0 warnings.
• node scripts/check-docs-governance.mjs — passed.
• git diff --check — clean.

Not verified here: the full solution test run (ci-required repeats it on this push), and
Record_IsOwnerReadWriteOnly_OnPosix, which returns early on this Windows box — the POSIX 0600
creation mode is proven only by the Linux CI leg.

The #2466 stderr constants are untouched, and nothing outside backend/src/Taskdeck.Cli, its test
project and that one doc bullet changed.

[Chris0Jeky]

Review (agent half of the gate; Codex credits exhausted, SC-9). Two fresh-context reviewer passes (read-only, Opus 5).

Round 1 at 7dcf503: verdict SHIP, no CRITICAL/HIGH. Confirmed the load-bearing property: a traversal-shaped reference could previously steer a record to <dataDirectory>/x.txt and is now refused before any filesystem call; the eviction reorder is arithmetically correct; the failed-write case deletes nothing. Two MEDIUMs and two LOWs were worth closing before merge because they concern the sink's own guarantees, so a fix round was taken.

Fix round at 1e1c600 (all four fixed, none declined): eviction runs in its own try/catch after the stream is closed, so an enumeration failure can no longer report a durable record as not captured (test through an internal record-lister seam); an attached --flag=value keeps only the flag name plus [value] before the redactor runs; the reference validator accepts hex in either case with the length constant now defined once in CliStartupTrace (a test asserts every reference IsCorrelationId accepts is accepted by the sink, and traversal is still refused); the self-eviction skip branch has a test. Sink and boundary tests 42/42, whole Cli project 222/222, solution build 0 errors, docs governance green.

Round 2 (scoped to the fix diff) at 1e1c600: verdict SHIP. Each finding traced closed; every attached-value shape (--flag=value, --flag=key=value, --flag=, -x=secret) renders as <flag>=[value]; the #2466 stderr constants are untouched. One residual MEDIUM: a value that itself begins with a dash (a card title such as - fix login for jane@acme.com, which ArgParser.GetOption accepts) was still classified as a flag name and retained verbatim, and the doc claimed titles stay off disk.

Closed in c7ca196 (a shape rule, no new review round owed): a flag name is a dash-prefixed token whose name part (before any =) contains no whitespace, so - fix login for jane@acme.com is replaced with [value] while --title=Secret plan still renders as --title=[value]; the doc names the one shape that stays ambiguous (a single dash-prefixed word used as a value is indistinguishable from a flag name and is retained). New test TryRecord_ReplacesADashLeadingValueThatContainsWhitespace; sink and boundary tests 43/43 at c7ca196.

Merge gate: hosted ci-required green at c7ca196 plus the aging floor.