Mark the faulty reference on the chain

The chain already said which objects are stuck and which belong in memory.
What it didn't say is which reference between them is the leak, which is the
one thing to go and change: a reader following the statuses is being pointed
at what the leak left behind rather than at the leak.

So the one step that goes from an Expected object straight to a Stuck one
reads "Holder.activity · faulty reference", bold, in the red of the objects
under it, and the hover says why that reference and not another.

Only that shape is marked. A stretch of several references between the two
verdicts is a fault at one of those steps with nothing saying which, and a
chain with nothing Expected above the stuck object can have its fault further
up than the path reaches — a chain of Cleaners nothing recognizes would
otherwise get its top reference marked for being where the walk started. So
the mark is either the answer or absent, and setting a verdict by hand is what
turns a stretch into one step.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Author: pyricau

docs/shark-explorer-changelog.md

@@ -38,3 +38,9 @@ uses, without the one for a newly recognized library leak:
   object as stuck makes it a leak and takes whatever it holds off the list. Kept between runs in
   `~/.shark-explorer/leak-statuses`, one file per heap dump.
   See [The verdict](shark-explorer.md#the-verdict).
+* ✨ **The chain marks the faulty reference**: the one step going from an `Expected` object straight to a
+  `Stuck` one reads `Holder.activity · faulty reference`, which is the leak itself rather than one of the
+  objects it left behind, and the same reference the **Leaks** screen names that leak after. A chain whose
+  two verdicts are further apart than one step carries no mark, since which reference in between is at fault
+  is what isn't known — overrule a verdict in between and the mark appears.
+  See [The verdict](shark-explorer.md#the-verdict).

docs/shark-explorer.md

@@ -165,8 +165,20 @@ and only the garbage collector not having run keeps it here. The verdict means t
 
 The reason is the rest of the answer, because half of these are about another object: an activity is red
 because its own `mDestroyed` is true, and the view under it is red because the activity is. `Activity↑ is
-stuck` is the chain saying so — and it is also how you find the faulty reference, which is the step between
-the last `Expected` object and the first `Stuck` one.
+stuck` is the chain saying so.
+
+**The chain marks the faulty reference itself**: `Holder.activity · faulty reference`, in bold red, on the one
+step that goes from an `Expected` object straight to a `Stuck` one. It is the one line of a chain that says
+where to go and change code — the shades on the objects are what the leak left behind, this is the leak — and
+it is the same reference the Leaks screen names that leak after, so a row there and the chain you open from it
+name one thing.
+
+**A chain with no such step carries no mark**, which is deliberate: what would be marked would be a guess
+drawn as an answer. With objects nothing knows either way about between the two verdicts, the fault is at one
+of those steps and nothing on the chain says which. With nothing `Expected` above the stuck object at all,
+what holds it may be something that should have let go of it too, so the fault can be further up than the
+chain reaches. Overruling a verdict is what closes either gap: say what you know about one object in between,
+and the mark appears on the step that leaves.
 
 **The pencil beside it** overrules the verdict. Pick one of the three, type why, and **Set the verdict**:
 

shark/shark-explorer/AGENTS.md

@@ -76,6 +76,19 @@ deliberately avoid "leak" on an object, since a leak is one faulty reference and
 leaking points readers at the wrong thing. `LeakStatus.statusText` is the only place the two meet, so change
 a word there and nowhere else. `notes/decisions.md` has why each word won.
 
+**Which reference the leak is, is decided once, over the whole path** — `faultyReferenceIndexOrNull`, called
+from `withLeakStatuses` — and carried on `PathReference.isFaulty` for the drawing to read. Working it out in
+the window from the steps on screen looks equivalent and isn't: a pane draws stretches of a chain
+(`stepsBelow`, `stepsAfter`, a swapped-in `RootPathWay`), so the object that ends the stretch can be above
+what it shows.
+
+**And it marks nothing unless one step crosses from `Expected` to `Stuck`.** `suspectReferenceIndexes`, the
+whole stretch between the two verdicts, is what a leak is *named* after — `suspectSubpath`, and Shark's leak
+fingerprint — so it is tempting to mark the top of it, which is what the first version did and what
+`notes/decisions.md` records as wrong: a chain whose objects all have no verdict got its top reference marked
+for being where the walk started. A longer stretch is a fault at one of several steps with nothing saying
+which, and nothing drawn is the answer for that.
+
 Someone reading a heap dump can overrule what the inspectors made of an object, and the statuses they set
 are a `LeakStatusOverrides` **passed into every question whose answer they change** — `summarize`,
 `rootPathTo`, `independentPathsBetween`, `independentPathsFromRoots`, `findLeaks`, `isBelowLeakingObject` —

shark/shark-explorer/notes/decisions.md

@@ -877,8 +877,26 @@ window.
 
 **"Faulty reference" is the name for the culprit**, the reference between the last `Expected` object and the
 first `Stuck` one, which is what `LeakGroup.suspectPath` starts at and what the leaks screen names each row
-after. Only prose so far — nothing on a chain marks that step yet, and marking it is the change that would
-actually put a reader's eye on the reference rather than on the objects.
+after. **And the chain marks it**: `Holder.activity · faulty reference`, bold, in the red of the objects it
+left behind, which is the change that actually puts a reader's eye on the reference rather than on the
+objects. `PathReference.isFaulty`, worked out in `withLeakStatuses`, and `suspectSubpath` names the leaks
+screen's rows off the same statuses — so where a leak is a single reference, a row there and the chain opened
+from it name one thing.
+
+**Only a single step between the two verdicts is marked.** `faultyReferenceIndexOrNull` asks for an `Expected`
+object with a `Stuck` one directly under it, and marks nothing otherwise. The first attempt marked the top of
+the suspect stretch instead, the way LeakCanary underlines all of it, and it was wrong in the case that
+matters: a chain of `Cleaner`s with no verdict on any object of it had its top reference marked, which is a
+reference named for being where the walk started rather than for anything read off the heap dump. Two shapes
+make the stretch longer than a step and neither supports a mark — objects nothing knows either way about in
+between, where the fault is at one of those steps and nothing says which; and nothing `Expected` above the
+stuck object at all, where what holds it may be something that should have let go too, so the fault can be
+further up than the path reaches. A guess drawn in the same bold red as an answer costs more than no mark,
+because being the one line to act on is the whole of what the mark is for. Shortening the stretch is what
+setting a verdict by hand does, and the mark appears when it becomes one step.
+
+**Nothing is marked on a chain with nothing stuck on it**, which is most chains in a heap dump. A leak is a
+reference the evidence points at, and there is no evidence until something below it is known not to belong.
 
 **A pencil, left of the status, rather than a "Set by hand…" button.** It is what changes the answer, so it
 belongs where the eye already is, and a text button pushed the reason onto a second line of a 320dp panel.

shark/shark-explorer/shark-explorer-app/src/main/java/shark/explorer/app/LeaksScreen.kt

@@ -218,7 +218,7 @@ private fun OnTheWayOutHeader(
     HorizontalDivider(thickness = SECTION_RULE_WIDTH, color = SECTION_RULE_COLOR)
     Column(
       Modifier.fillMaxWidth()
-        .clickableRow(onToggle)
+        .clickableRow(onClick = onToggle)
         .padding(start = 8.dp, end = 12.dp, top = 6.dp, bottom = 6.dp),
       verticalArrangement = Arrangement.spacedBy(2.dp)
     ) {
@@ -622,7 +622,9 @@ internal const val NAME_HINT =
     "which is where to look on the chain to see it. They are one reference for most leaks. Everything " +
     "above the first is the app working as intended and everything below the last is what the leak is " +
     "holding, so neither is part of what makes this leak this leak. For an object on its way out it is " +
-    "the one reference the collector hasn't cleared yet, which is the whole of why it is still here."
+    "the one reference the collector hasn't cleared yet, which is the whole of why it is still here. Where " +
+    "a leak is a single reference, the chain drawn for an object under this row marks it, so the row and " +
+    "the step are one thing said twice."
 
 internal const val LEAK_FINGERPRINT_HINT =
   "A hash of how this leak is held, which is the same for the same leak in the next heap dump of this app " +

shark/shark-explorer/shark-explorer-app/src/main/java/shark/explorer/app/PathDrawing.kt

@@ -457,6 +457,12 @@ private fun ReferenceLine(reference: PathReference) {
         ) {
           append(reference.displayName())
         }
+        // The one line of a chain that says where to go and change code, so it is the one thing on a
+        // chain drawn bold: the shades on the objects say what a leak left behind, and this says what the
+        // leak is. See [shark.explorer.PathReference.isFaulty].
+        if (reference.isFaulty) {
+          withStyle(FAULTY_REFERENCE_SPAN) { append(" $FAULTY_REFERENCE") }
+        }
         // Which is what makes a chain through a known leak readable as one: the objects below this
         // reference are held by code the app doesn't control, and this is the reference that does it.
         if (reference.libraryLeak != null) {
@@ -466,12 +472,16 @@ private fun ReferenceLine(reference: PathReference) {
       style = MaterialTheme.typography.bodySmall
     )
   }
-  // What is known about the leak is a paragraph, which belongs on hover rather than in the chain.
-  val description = reference.libraryLeak?.description?.takeIf { it.isNotEmpty() }
-  if (description == null) {
+  // Why this reference and not another, and what is known about a leak somebody else's code holds: both are
+  // paragraphs, so both belong on hover rather than in the chain.
+  val explanation = listOfNotNull(
+    FAULTY_REFERENCE_HINT.takeIf { reference.isFaulty },
+    reference.libraryLeak?.description?.takeIf { it.isNotEmpty() }
+  )
+  if (explanation.isEmpty()) {
     line()
   } else {
-    Hint(description, line)
+    Hint(explanation.joinToString("\n\n"), line)
   }
 }
 
@@ -628,6 +638,20 @@ private const val LOCAL_VARIABLE = "<local variable>"
 /** What a reference Shark knows leaks in code the app doesn't control says about itself. */
 internal const val LIBRARY_LEAK = "· known library leak"
 
+/**
+ * And what the reference the leak is says about itself, which is the one thing on a chain to go and fix.
+ *
+ * Two words rather than a sentence, in the red of the objects it left behind: a chain is read as a column
+ * of names, and this is the line to stop on.
+ */
+internal const val FAULTY_REFERENCE = "· faulty reference"
+
+/** Why this reference of the chain and not another, which is a paragraph and so sits on hover. */
+internal const val FAULTY_REFERENCE_HINT =
+  "The leak itself: what this reference is read on is expected to be in memory, what it points at should " +
+    "have been gone, so this is the one reference that should have been cleared. Everything under it is " +
+    "only still here because of it, and clearing this one is what would let all of it go."
+
 /** Wide enough for the line, its arrow head and a ring to sit clear of the text beside it. */
 private val GUTTER_WIDTH = 26.dp
 
@@ -721,6 +745,9 @@ private val MUTED_SPAN = SpanStyle(color = MUTED_TEXT, fontWeight = FontWeight.N
 /** And for the words saying a reference is a known library leak, in the red of the leaks it explains. */
 private val LIBRARY_LEAK_SPAN = SpanStyle(color = LEAKING_TEXT, fontWeight = FontWeight.Normal)
 
+/** The same red for the reference the leak is, bold: nothing else on a chain is the thing to fix. */
+private val FAULTY_REFERENCE_SPAN = SpanStyle(color = LEAKING_TEXT, fontWeight = FontWeight.Bold)
+
 /** The letter drawn in an object's circle, which is what kind of object it is. */
 private val HeapObjectKind.badgeLetter: String
   get() = when (this) {

shark/shark-explorer/shark-explorer-app/src/test/java/shark/explorer/app/LeakStatusSectionTest.kt

@@ -39,12 +39,13 @@ import shark.explorer.statusText
 
 /**
  * Whether the object a tab is on is meant to be in memory, said at the top of the panel that says what the
- * object is, and changed by hand from there.
+ * object is, changed by hand from there, and what the answer marks on the chain beside it.
  *
  * What the statuses mean and how two of them disagree is `LeakStatusTest` and `HeapLeakStatusTest` in
  * `shark-explorer-core`, and where they are kept is `LeakStatusFileTest`. What is only true here is that the
- * panel says what the heap dump says, that changing one asks for the reason before it writes anything, and
- * that a status which cannot be true alongside another is shown rather than settled quietly.
+ * panel says what the heap dump says, that changing one asks for the reason before it writes anything, that
+ * a status which cannot be true alongside another is shown rather than settled quietly, and that the chain
+ * says which reference the leak is.
  */
 @OptIn(ExperimentalTestApi::class)
 class LeakStatusSectionTest {
@@ -207,6 +208,34 @@ class LeakStatusSectionTest {
     }
   }
 
+  /**
+   * What the verdicts are for: they are about objects, and the thing to go and fix is the one reference going
+   * from an object that belongs in memory to one that doesn't.
+   *
+   * Both halves in one window, because they are one answer: the reference is marked from the verdicts either
+   * side of it, so a verdict set by hand is what puts the mark on the chain and what takes it off again.
+   */
+  @Test fun `the chain marks which reference the leak is, and a hand can take the mark off`() {
+    explorerUiTest {
+      // Set in a run before this one, and what leaves a single reference below it: with nothing on this
+      // chain known to belong in memory, the fault is at either of its two steps and neither is marked.
+      openHeapDump(setAlready = { holderIsExpected() }) { it.activityObjectId }
+
+      onNodeWithText("$FAULTY_STEP $FAULTY_REFERENCE").assertIsDisplayed()
+
+      changeStatus()
+      choose(LeakStatus.NOT_LEAKING)
+      write(TYPED_REASON)
+      set()
+
+      // Nothing on this chain is stuck any more, so there is no reference to point at — and the step is
+      // still drawn, which is the mark being about the leak rather than about the reference.
+      waitUntilAtLeastOneExists(hasText(TYPED_REASON, substring = true), SAVE_TIMEOUT_MILLIS)
+      onNodeWithText(FAULTY_REFERENCE, substring = true).assertDoesNotExist()
+      onNodeWithText(FAULTY_STEP).assertIsDisplayed()
+    }
+  }
+
   /**
    * The other half of setting a status: the leaks are read through them, so the list changes rather than
    * only the colour of one object. See [shark.explorer.HeapDominatorTreemap.findLeaks].
@@ -302,14 +331,22 @@ class LeakStatusSectionTest {
   }
 
   /** A status set on the holder in a run before the one under test, which is the file being there. */
-  private fun holderIsLeaking() {
+  private fun holderIsLeaking() = holderWasSetTo(LeakStatus.LEAKING, HOLDER_REASON)
+
+  /** And the other way: a holder that belongs in memory, with the activity below it still stuck. */
+  private fun holderIsExpected() = holderWasSetTo(LeakStatus.NOT_LEAKING, HOLDER_EXPECTED_REASON)
+
+  private fun holderWasSetTo(
+    status: LeakStatus,
+    reason: String
+  ) {
     statusFile().write(
       LeakStatusOverrides.of(
         listOf(
           LeakStatusOverride(
             objectId = heapDump.holderObjectId,
-            status = LeakStatus.LEAKING,
-            reason = HOLDER_REASON
+            status = status,
+            reason = reason
           )
         )
       )
@@ -352,9 +389,15 @@ class LeakStatusSectionTest {
     /** And what the object holding it is called, where the dialog names that one. */
     private const val HOLDER_NAME = "Holder instance"
 
+    /** The step of the chain that holds the destroyed activity, which is the reference to clear. */
+    private const val FAULTY_STEP = "Holder.activity"
+
     /** And what it was given as its reason, which the dialog has to show to be overruled. */
     private const val HOLDER_REASON = "this holder is the one to fix"
 
+    /** The reason for the other status a run before this one set on the holder. */
+    private const val HOLDER_EXPECTED_REASON = "this holder is the app's own cache"
+
     /** What the dialog says about a status set on an object that holds the one being changed. */
     private const val CONFLICT_ABOVE = "holds it"
 

shark/shark-explorer/shark-explorer-core/src/main/java/shark/explorer/DominatorPaths.kt

@@ -125,7 +125,23 @@ data class PathReference(
   val ownerClassName: String,
   val locationType: ReferenceLocationType,
   /** Set for the references Shark knows leak in code an app doesn't control, null for the rest. */
-  val libraryLeak: LibraryLeakPattern?
+  val libraryLeak: LibraryLeakPattern?,
+  /**
+   * Whether this is the reference the leak *is*: the one step of the path that goes from an object expected
+   * to be in memory to a stuck one.
+   *
+   * **The one thing on a chain that says where to go and change code.** A status is about an object, and
+   * every object below this reference reads as stuck because of it — so a reader following the statuses is
+   * being pointed at what a leak left behind, and this is being pointed at the leak. The same reference the
+   * leaks screen names a leak after, wherever a leak is a single reference. See
+   * [faultyReferenceIndexOrNull], which is where the rule and the three ways a path has no faulty reference
+   * are, and [LeakGroup.suspectPath].
+   *
+   * False for every reference of most paths of a heap dump, since it takes the two verdicts either side of
+   * one reference to be true. Worked out once the whole path is known, like [PathStep.leakStatus], for the
+   * same reason: the statuses of the objects either side of it are what decide it.
+   */
+  val isFaulty: Boolean = false
 )
 
 /**