🩹 fix: Address comprehensive-review findings on attachment helpers

Five findings from the latest pass:

- **MAJOR — `displayFilename` false-positive on extensionless 6-hex.**
  The previous regex `/-[0-9a-f]{6}(?=\.[^.]+$|$)/` stripped any leaf
  ending in `-XXXXXX` regardless of context, so a user-named
  `build-a1b2c3` (script-emitted hash artifact, no extension) lost its
  tail and rendered as `build`. Split into two narrower patterns:
  `COLLISION_SUFFIX_BEFORE_EXT` only matches when followed by an
  extension; `SANITIZED_DOTFILE_TRAILING_SUFFIX` only fires when the
  leaf starts with `_.` AND ends with `-XXXXXX` — the unambiguous
  fingerprint of `sanitizeArtifactPath`'s dotfile rewrite.

- **MINOR — `isInternalSandboxArtifact` filter too aggressive.**
  `(file.bytes ?? 0) > 0` treated undefined bytes as zero, falling
  through to the regex check. Tightened to `file.bytes !== 0`: only
  an *explicit* zero counts as the empty-placeholder shape worth
  hiding. Non-code-exec sources without `bytes` populated render
  normally now.

- **MINOR — `getValue()` could throw on a degenerate atom state.**
  Switched the snapshot read in `ToolArtifactCard` to
  `valueMaybe() ?? false` so a transient error / loading state on the
  upstream selector doesn't crash card mount. The `false` default is
  the right history-fallback (don't auto-open if we can't classify).

- **NIT — `attachmentSalience` / `bySalience` over-broad signature.**
  Removed the test-only `{ bytes?: number }` arm; functions now accept
  `TAttachment` directly. The internal `bytes` read still goes through
  a cast since not every TAttachment branch declares it. Tests updated
  to use the existing `baseAttachment(...)` helper.

- **MINOR — Missing regression test for extensionless 6-hex.**
  Added `'build-a1b2c3'` and `'out/blob-deadbe'` cases that pin the
  preservation behavior, plus an `isInternalSandboxArtifact` test that
  asserts undefined-bytes attachments are not filtered.

client/src/components/Chat/Messages/Content/Parts/ToolArtifactCard.tsx

@@ -86,7 +86,13 @@ const ToolArtifactCard = memo(({ attachment, artifact }: ToolArtifactCardProps)
   const readInitialIsSubmitting = useRecoilCallback(
     ({ snapshot }) =>
       () =>
-        snapshot.getLoadable(store.isSubmittingFamily(0)).getValue(),
+        // `valueMaybe()` returns `undefined` if the atom is in an error
+        // or loading state instead of throwing — defensive against an
+        // upstream selector failure surfacing during card mount. The
+        // `?? false` default is correct because a card we can't classify
+        // as streaming is one we should treat as history (don't steal
+        // focus / open the panel).
+        snapshot.getLoadable(store.isSubmittingFamily(0)).valueMaybe() ?? false,
     [],
   );
   const mountedDuringStreamRef = useRef<boolean | null>(null);

client/src/components/Chat/Messages/Content/Parts/__tests__/attachmentTypes.test.ts

@@ -205,6 +205,16 @@ describe('isInternalSandboxArtifact', () => {
     } as Partial<TAttachment>);
     expect(isInternalSandboxArtifact(attachment)).toBe(false);
   });
+
+  it('does NOT match when bytes is undefined (size unknown ≠ empty)', () => {
+    /** Non-code-exec attachment sources can omit `bytes` entirely. The
+     * filter only fires for an *explicit* zero-byte placeholder so we
+     * don't accidentally hide files we don't have size info for. */
+    const attachment = baseAttachment({
+      filename: '_.dirkeep-88b30b',
+    } as Partial<TAttachment>);
+    expect(isInternalSandboxArtifact(attachment)).toBe(false);
+  });
 });
 
 describe('displayFilename', () => {
@@ -239,27 +249,46 @@ describe('displayFilename', () => {
     // 7 chars after the dash → not the canonical 6-char hash form.
     expect(displayFilename('build-1234567.log')).toBe('build-1234567.log');
   });
+
+  it('preserves an extensionless leaf that ends in 6 hex chars', () => {
+    /** The dotfile-anchored fallback only fires when the leaf starts
+     * with `_.` AND ends with `-XXXXXX`. A user-named extensionless
+     * file like `build-a1b2c3` has no `_.` prefix, so neither pattern
+     * matches and the name is preserved. Without that anchor an
+     * agent-emitted hash-named output would silently lose its tail. */
+    expect(displayFilename('build-a1b2c3')).toBe('build-a1b2c3');
+  });
+
+  it('preserves an extensionless leaf with hex suffix inside a directory', () => {
+    expect(displayFilename('out/blob-deadbe')).toBe('out/blob-deadbe');
+  });
 });
 
 describe('attachmentSalience', () => {
   it('returns 0 for non-empty content (sorts first)', () => {
-    expect(attachmentSalience({ bytes: 47 })).toBe(0);
+    expect(attachmentSalience(baseAttachment({ bytes: 47 } as Partial<TAttachment>))).toBe(0);
   });
 
   it('returns 1 only for an explicit zero-byte entry (sinks last)', () => {
-    expect(attachmentSalience({ bytes: 0 })).toBe(1);
+    expect(attachmentSalience(baseAttachment({ bytes: 0 } as Partial<TAttachment>))).toBe(1);
   });
 
   it('treats undefined `bytes` as neutral so non-code-exec sources do not silently sink', () => {
     /** Web-search results, uploaded files where the schema omits `bytes`,
      * etc. should keep their input order — only an explicit `bytes === 0`
      * counts as the empty-placeholder shape we want to demote. */
-    expect(attachmentSalience({})).toBe(0);
+    expect(attachmentSalience(baseAttachment({}))).toBe(0);
   });
 
   it('produces a stable bucket sort when used as `(a,b) => salience(a) - salience(b)`', () => {
-    const real = { bytes: 47, filename: 'test_file.txt' };
-    const placeholder = { bytes: 0, filename: '_.dirkeep-88b30b' };
+    const real = baseAttachment({
+      bytes: 47,
+      filename: 'test_file.txt',
+    } as Partial<TAttachment>);
+    const placeholder = baseAttachment({
+      bytes: 0,
+      filename: '_.dirkeep-88b30b',
+    } as Partial<TAttachment>);
     const sorted = [placeholder, real].sort(
       (a, b) => attachmentSalience(a) - attachmentSalience(b),
     );

client/src/components/Chat/Messages/Content/Parts/attachmentTypes.ts

@@ -20,14 +20,25 @@ import { detectArtifactTypeFromFile } from '~/utils/artifacts';
 const SANDBOX_PLACEHOLDER_LEAVES = /^_\.(?:dirkeep|gitkeep)-[0-9a-f]{6}$/i;
 
 /**
- * Drop the deterministic 6-hex disambiguator the backend appends when
- * sanitization mutated the raw filename (e.g. `.dirkeep` → `_.dirkeep-88b30b`,
- * `out 1.csv` → `out_1-<hash>.csv`). The hash is collision-avoidance
- * machinery; users only need to see a recognizable name. We strip it
- * for display *only* — the on-disk filename keeps the suffix so
- * downloads still resolve.
+ * Two complementary patterns recover the user-visible filename from the
+ * backend's sanitized form. Splitting them avoids the false-positive
+ * where a legitimate filename happens to end in `-` + 6 hex chars:
+ *
+ *   - `COLLISION_SUFFIX_BEFORE_EXT` matches the suffix only when an
+ *     extension follows (`output-deadbe.csv` → `output.csv`). This is
+ *     the broad case from `embedDisambiguatorInLeaf` for non-dotfiles
+ *     where sanitization mutated something (spaces, special chars).
+ *
+ *   - `SANITIZED_DOTFILE_TRAILING_SUFFIX` matches only when the leaf
+ *     starts with `_.` AND ends with `-XXXXXX`. That combination is
+ *     the unambiguous fingerprint of `sanitizeArtifactPath`'s dotfile
+ *     rewrite (`.dirkeep` → `_.dirkeep-88b30b`). Without this anchor,
+ *     a user-named `build-a1b2c3` would lose its `-a1b2c3` suffix
+ *     because there's no way to tell intent from a hex-shaped tail
+ *     alone.
  */
-const COLLISION_SUFFIX = /-[0-9a-f]{6}(?=\.[^.]+$|$)/;
+const COLLISION_SUFFIX_BEFORE_EXT = /-[0-9a-f]{6}(?=\.[^.]+$)/;
+const SANITIZED_DOTFILE_TRAILING_SUFFIX = /^(_\..+)-[0-9a-f]{6}$/;
 
 /**
  * Last segment of a forward-slash path. The backend stores filenames as
@@ -43,10 +54,15 @@ const leafOf = (filename: string | undefined): string => {
 /**
  * `true` when the attachment is a sandbox-internal placeholder (empty
  * folder marker) the user shouldn't see as its own file chip.
+ *
+ * `bytes` must be *explicitly* zero to qualify — `undefined` means
+ * "size unknown" (web-search results, archived attachments where the
+ * schema omits the byte count) and we render those normally rather
+ * than hiding them on a name match alone.
  */
 export const isInternalSandboxArtifact = (attachment: TAttachment): boolean => {
   const file = attachment as TFile & TAttachmentMetadata;
-  if ((file.bytes ?? 0) > 0) {
+  if (file.bytes !== 0) {
     return false;
   }
   return SANDBOX_PLACEHOLDER_LEAVES.test(leafOf(attachment.filename));
@@ -66,7 +82,17 @@ export const displayFilename = (filename: string | undefined): string => {
   const slash = raw.lastIndexOf('/');
   const dir = slash < 0 ? '' : raw.slice(0, slash);
   const leaf = slash < 0 ? raw : raw.slice(slash + 1);
-  const cleanedLeaf = leaf.replace(COLLISION_SUFFIX, '');
+  // Try the broad case first (suffix before an extension). If nothing
+  // matched, fall back to the dotfile-specific anchor so we don't strip
+  // a 6-hex tail off an extensionless leaf the user actually named that
+  // way (e.g. `build-a1b2c3` from a hash-named build artifact).
+  let cleanedLeaf = leaf.replace(COLLISION_SUFFIX_BEFORE_EXT, '');
+  if (cleanedLeaf === leaf) {
+    const dotfileMatch = leaf.match(SANITIZED_DOTFILE_TRAILING_SUFFIX);
+    if (dotfileMatch) {
+      cleanedLeaf = dotfileMatch[1];
+    }
+  }
   // Drop the leading-dotfile underscore prefix (`_.dirkeep` → `.dirkeep`)
   // only when paired with the collision suffix, since that combination is
   // a strong signal the underscore was added by sanitization. Standalone
@@ -86,24 +112,23 @@ export const displayFilename = (filename: string | undefined): string => {
  * omits the byte count) from silently sinking past real content. The
  * filter is intentionally narrow: only an explicit zero counts as empty.
  *
- * Accepts the broad `TAttachment` union (some branches lack `bytes`)
- * plus the bare `{ bytes?: number }` shape the unit tests use.
+ * The internal cast is needed because `TAttachment` is a union and only
+ * its first arm declares `bytes`; the function reads through the cast
+ * and treats `undefined` as neutral, which is correct for every arm.
  */
-export const attachmentSalience = (item: TAttachment | { bytes?: number }): number => {
+export const attachmentSalience = (item: TAttachment): number => {
   const bytes = (item as { bytes?: number }).bytes;
   return bytes === 0 ? 1 : 0;
 };
 
 /**
- * Stable comparator for arrays of `TAttachment`-like values. Equivalent
- * to `(a, b) => attachmentSalience(a) - attachmentSalience(b)` but
+ * Stable comparator for arrays of `TAttachment` values. Equivalent to
+ * `(a, b) => attachmentSalience(a) - attachmentSalience(b)` but
  * exported once so the lambda doesn't need to be repeated at every
  * call site.
  */
-export const bySalience = (
-  a: TAttachment | { bytes?: number },
-  b: TAttachment | { bytes?: number },
-): number => attachmentSalience(a) - attachmentSalience(b);
+export const bySalience = (a: TAttachment, b: TAttachment): number =>
+  attachmentSalience(a) - attachmentSalience(b);
 
 /**
  * Comparator variant for buckets that wrap the attachment in a record