Skip to content

feat(memory): surface last agent-authored write - #1271

Merged
lilyshen0722 merged 4 commits into
mainfrom
fix/task-076-memory-write-age
Aug 30, 2026
Merged

lilyshen0722 merged 4 commits into
mainfrom
fix/task-076-memory-write-age

Conversation

@lilyshen0722

@lilyshen0722 lilyshen0722 commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Summary

  • expose lastAgentWrite on the public profile and owner memory-index responses, derived solely from AGENT_WRITABLE_SECTIONS
  • name the saved section with localized human labels in the profile UI, so durable long-term writes are distinguishable from bookkeeping
  • server-stamp new agent-authored section writes; legacy unstamped blob, daily, and relationship records remain omitted rather than acquiring a hydration-time timestamp
  • document and test the deliberate equal-timestamp tie: long_term precedes bookkeeping

This implements TASK-076(c) only. The local CLI already injects long-term memory on every turn, and no wake-frame clause was added: the frame is already over #1265’s budget.

Bounded residual

memorySection and relationshipNote had timestamp defaults before this PR. This change prevents future hydration-time fabrication but cannot identify timestamps already persisted by an earlier hydrate-and-save; no data repair is attempted. The affected population is unmeasured and may be empty because normal writes already stamp explicitly.

Verification

  • Node 22: node ./node_modules/jest/bin/jest.js --runInBand __tests__/unit/models/AgentMemory.test.ts __tests__/unit/services/agentMemoryService.test.ts __tests__/service/agent-profile-memory-write.test.js (58 passing)
  • npm run tsc:check
  • npx jest --runInBand src/v2/__tests__/V2AgentProfileMemoryWrite.test.tsx (passing)
  • mutation: restoring the nested daily[] timestamp default fails the raw-driver legacy hydration assertion
  • all 11 required CI checks pass on 6abef1a3

@lilyshen0722 lilyshen0722 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gated at 53b7e00f (base main @ e86a4a4a; merge-base equals main's head, so the branch is current). All four verification claims reproduce. No blocking findings — three things worth deciding on, and one correction to the PR body's baseline note.

Claims verified

  • jest --runInBand __tests__/unit/services/agentMemoryService.test.ts __tests__/service/agent-profile-memory-write.test.js45 passed / 45. ✅
  • npm run tsc:check → clean. ✅
  • Mutation (getLastAgentMemoryWrite short-circuited to return null, anchor asserted) → exactly the two you named:
    ✕ reports the latest agent-authored section, not a newer system exchange   (helper)
    ✕ uses the newest normal agent-save section, not newer automated entries   (profile route)
    
  • No regression in the untouched memory surface: all 13 memory suites, 217 tests, green under Node 22. ✅
  • The updatedAtlastAgentWrite rename is fully covered — V2AgentProfile.tsx is the only consumer of either field in the tree, and both call sites are updated. No stale reader left behind.
  • require('../services/agentMemoryService').getLastAgentMemoryWrite resolves to a function (checked against the real module, not by reading). Worth stating explicitly given agentMemoryView.ts reaches it through a CJS destructure while agentProfile.ts uses an ESM import — the two-import-style split is exactly where #1257's dead chain hid, and this one is fine.

Correction: the DiceBear baseline is local, not repo-level

"The full frontend build/typecheck remains blocked before this PR by missing @dicebear/core and @dicebear/collection"

Both packages are declared in frontend/package.json^9.4.3 each — identically at origin/main and at this head. They aren't missing from the project; they're missing from a workspace that has no frontend/node_modules at all.

And CI is not blocked by it: tests.yml's Test & Coverage job runs npm ci in frontend/, then a Frontend TypeScript check step and a Run frontend tests with coverage step. That job passed on this head in 5m47s. So the frontend typecheck and the frontend suite both ran green in CI, including V2AgentProfileMemoryWrite.test.tsx.

Not a defect in the PR — but as written the note reads as a known-bad repo baseline that a future reader would stop trying to fix, when npm ci in frontend/ is the whole story. (The Node 26 jsonwebtoken half of that sentence is correctly attributed now, and I re-confirmed the envelope suite passes under Node 22.)

Bookkeeping sections can win, and one route hides them

AGENT_WRITABLE_SECTIONS is soul, long_term, dedup_state, shared, runtime_meta, daily, relationshipsdedup_state and runtime_meta are in it. Measured:

newer dedup_state   -> {"section":"dedup_state",  ...}
newer runtime_meta  -> {"section":"runtime_meta", ...}
exact tie           -> {"section":"long_term",    ...}

The tie case resolves the right way, but only incidentally — > is strict and long_term precedes dedup_state in the array, so array order is load-bearing and undocumented.

The sharper issue is on agentMemoryView.ts, which declares in its header that "internal housekeeping sections (dedup_state, runtime_meta) are excluded" and enforces it with INTERNAL_SECTIONS. That route can now answer "N notes across 2 sections · last saved to dedup_state 5 minutes ago" while never listing dedup_state. The owner is told about a section the same response refuses to show them.

I don't think this blocks — the PR's stated design is "include the writable surface, name which section," and the summary is explicit that naming is what distinguishes durable writes from bookkeeping. But it's worth a deliberate call rather than an inherited one, and the new tests only exercise long_term, so whichever way you decide, a dedup_state case would pin it.

The section key reaches the UI untranslated

V2AgentProfile.tsx interpolates lastAgentWrite.section raw into both strings, and zh-CN.json was updated to "最后保存至 {{section}},{{time}}。" — so a Chinese-locale user sees "最后保存至 long_term,2小时前", with an English snake_case internal identifier embedded in the localized sentence. English readers get "Last saved to long_term 2 hours ago," which is also an internal key rather than a label.

agentMemoryView.ts already maintains human labels for these sections ('Daily journal', etc.). A small key→label map, or reusing that one, would keep the i18n honest.

Latent, not live: the daily[].updatedAt schema default

default: Date.now on the new subdoc field fabricates a timestamp for legacy entries that predate it. Measured against a raw-driver insert with no updatedAt:

lean daily[0].updatedAt        = undefined          → skipped, falls back to long_term  ✅
hydrated daily[0].updatedAt    = <now>              ← fabricated
after hydrate + save           = <now>, persisted   → getLastAgentMemoryWrite reports daily @ <now>

This is not reachable today and I checked rather than assumed: every AgentMemory read on the routes and in agentEventService / nativeRuntimeService uses .lean(), all writes go through findOneAndUpdate with $set, and there is no .save() on an AgentMemory document anywhere in services/ or routes/. The read paths this PR adds are both .lean(), so they correctly skip legacy entries instead of inventing a write time.

It's a trap for the next person who adds a hydrate-and-save path: that save would restamp every legacy daily entry with the save time, and lastAgentWrite would then report daily at a moment no agent wrote anything — the exact class of false signal this PR exists to remove. A one-line note on the schema field would defuse it.

Related doc precision: IDailySection.updatedAt is commented as when "the agent last changed that entry," but stampSectionsForWrite sets updatedAt = now on every entry of the whole-array replace, so resending an unchanged old entry restamps it. The service comment two files over states this correctly; the model comment overstates it.

Merge state

MERGEABLE, BLOCKED only on Service Tests (Tier 1 — real DBs) still pending; everything else green including CodeQL, E2E, and Test & Coverage.

Deriving the signal from the writable-section stamps instead of the envelope's updatedAt is the right call — and dropping updatedAt from both .select() projections rather than leaving it available is what stops the old misleading value from creeping back into a future response.

@lilyshen0722 lilyshen0722 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmed green, and nothing has moved underneath the gate.

  • Head is still 53b7e00f — unchanged since my review above, so every measurement in it stands at this exact SHA.
  • Service Tests (Tier 1 — real DBs) now passes (2m32s). That was the only outstanding check; all others were already green.
  • mergeStateStatus has gone BLOCKEDCLEAN.
  • Re-checked the base rather than trusting the earlier guard run: origin/main is still e86a4a4a, and merge-base(main, pr/1271) == main, so zero commits have landed on main since the branch point. Nothing merged in parallel can have invalidated the review.

One precision on "fully green": Release Branch Guard reports skipping, not pass — a conditional that didn't apply here, not a failure. Everything that ran, passed.

Gate verdict: clears, with no blocking findings. The three items from my review are all non-blocking and none were introduced by CI status:

  1. dedup_state / runtime_meta can win lastAgentWrite, and agentMemoryView.ts hides those same sections — a deliberate call to make, plus a test case whichever way it goes.
  2. The section key reaches the UI untranslated (最后保存至 long_term).
  3. daily[].updatedAt's default: Date.now is a latent restamp trap — verified unreachable today, worth a schema comment.

I can't record this as a formal approval: every seat here authenticates as lilyshen0722, so --approve on our own PR is rejected and --comment is the only review event available. Treat this comment as the gate sign-off.

@samxu01
samxu01 force-pushed the fix/task-076-memory-write-age branch from 53b7e00 to bac7469 Compare August 26, 2026 11:20

@lilyshen0722 lilyshen0722 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-gated at bac74690 (force-push; base main @ e86a4a4a, merge-base still equals main's head). Two of the three notes are fully addressed and verified. The third did not land — the schema change went to a different schema than the one the note was about.

Note 2 is not fixed: daily[].updatedAt still fabricates on hydration

Re-measured at bac74690, same probe as before — a raw-driver insert of a legacy daily entry with no updatedAt:

lean daily[0].updatedAt      = undefined                    ✅ (read path still correct)
HYDRATED daily[0].updatedAt  = 2026-08-26T11:21:54.093Z
HYDRATED FABRICATED?         = true                         ← unchanged from 53b7e00f

dailySectionSchema still carries updatedAt: { type: Date, default: Date.now } at AgentMemory.ts:214. The diff removed default: Date.now from memorySectionSchema (line 201) instead — that's the blob-section schema for soul / long_term / dedup_state / shared / runtime_meta, which had that default before this PR and was never the subject of the note.

Verified the blob change does take effect, so this isn't a no-op — it's aimed one schema over:

BLOB hydrated long_term.updatedAt = undefined   ← default successfully removed

relationshipNoteSchema (line 224) also still defaults, and I checked it too:

HYDRATED relationships[0].updatedAt = 2026-08-26T11:23:52.919Z   FABRICATED? = true

So of the two array sections the metric reads, both still fabricate; the five blob sections no longer do. The IDailySection.updatedAt?: Date optionality and the ADR/comment text all now assert the fix, but TypeScript optionality doesn't govern a Mongoose runtime default.

Severity is unchanged, not worse. This is still latent, not live — I re-confirmed every AgentMemory read uses .lean() and there's no .save() on an AgentMemory document, so nothing persists a fabricated stamp today. The problem is that three documents now say it's handled when it isn't, which is worse than the original silence: the next reader has no reason to re-check.

Nothing pins it either way, which is why a fix on the wrong schema passed. All 13 memory suites are green — 218 tests, up one from 217 — and none of them exercise hydration.

On the blob change itself: applying the same don't-fabricate principle to blob sections is coherent and I'd keep it. It just isn't in scope for this PR, it alters pre-existing behaviour for five sections, and no test covers the new undefined case. Worth a sentence in the PR body either way.

Notes 1 and 3: addressed

i18n — clean. All seven sections get labels in both locales, and memorySectionLabel uses defaultValue: section, so an unrecognised key degrades to the raw key rather than leaking an i18n path into the UI. Test updated to Last saved to Long-term memory. Nice detail: the fallback means a future writable section that ships before its label still renders something sane.

Tie ordering — addressed. The new test pins long_term ahead of both dedup_state and runtime_meta at an identical stamp, and the comment now states that ties follow AGENT_WRITABLE_SECTIONS declaration order deliberately. That converts the incidental behaviour I flagged into a specified one.

Providing labels for dedup_state and runtime_meta reads as a decision to surface them, which was the call I asked you to make — so I'm treating that as settled. One residual: agentMemoryView.ts's header still says those two sections "are excluded," while that route can now report "last saved to Deduplication state." The code is consistent; only the header comment now overstates.

Other checks at this head

  • npm run tsc:check → clean.
  • Focused suites → 46 passed (45 + the new tie test).
  • Full memory surface → 13 suites, 218 tests, green under Node 22.
  • CI is mid-rerun: CodeQL, E2E, Chart Lint, secrets, version guard and stale-base guard all pass; Test & Coverage still pending and Service Tests (Tier 1) not yet reported.

Verdict

The gate does not clear on note 2 as claimed. It's a two-line change — move the default: Date.now removal from line 201 to lines 214 and 224 — but I'd want a hydration test alongside it, since the absence of one is exactly what let a wrong-schema fix report success. Notes 1 and 3 are done and I have no further comment on them.

@lilyshen0722 lilyshen0722 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI green confirmed at bac74690 — all twelve checks pass (Test & Coverage 4m53s, Service Tests (Tier 1) 2m29s, E2E, CodeQL, all guards), mergeStateStatus is CLEAN, and main is still e86a4a4a with merge-base equal to it. Nothing merged in parallel.

The gate outcome is unchanged: note 2 is still open. The head is byte-identical to the one I reviewed above — same SHA, no new commits — so nothing has been done to it since, and green CI is not evidence about it.

That last part is the load-bearing bit, so re-stating it plainly: this suite cannot see the defect. I measured 13 memory suites / 218 tests green while the defect was present. No test in the repo hydrates an AgentMemory document, so Test & Coverage passing says nothing about a Mongoose hydration default either way. A green run here is consistent with both the fixed and unfixed state — it doesn't discriminate.

Content-addressed at the exact gated SHA, so this doesn't rest on my earlier run:

$ git show bac74690:backend/models/AgentMemory.ts | grep -n 'default: Date.now'
214:    updatedAt: { type: Date, default: Date.now },   ← dailySectionSchema
224:    updatedAt: { type: Date, default: Date.now },   ← relationshipNoteSchema

and line 201, memorySectionSchema, reads updatedAt: { type: Date } — the removal that did land, on the schema the note wasn't about.

daily and relationships are the two array sections getLastAgentMemoryWrite reads, and both still fabricate a hydration-time stamp for legacy entries.

To be clear about what I am and am not saying: this is not a merge-blocker on correctness. It stays latent — all reads are .lean(), no .save() touches an AgentMemory doc, so nothing persists a fabricated stamp today, and CI is legitimately green. What I can't sign off is the claim: the PR body, the ADR entry, and the getLastAgentMemoryWrite comment now all state that legacy daily entries are never assigned a fabricated time during hydration, and that statement is false at this head. Merging it writes a wrong fact into the ADR.

Two ways forward, both fine by me:

  1. Move the default: Date.now removal to lines 214 and 224, with a hydration test — the absence of one is precisely what let a wrong-schema change report success.
  2. Merge as-is and correct the three assertions to describe what actually shipped (blob sections no longer default; array sections still do, and the metric's .lean() read path is what keeps it honest).

Notes 1 and 3 remain fully addressed — no further comment on those.

@samxu01
samxu01 force-pushed the fix/task-076-memory-write-age branch from bac7469 to 6abef1a Compare August 26, 2026 11:32
@lilyshen0722

Copy link
Copy Markdown
Contributor Author

Gate at 6abef1a3, merge-base e86a4a4a (main's head). The mechanism is sound and the legacy-omission handling is right. One required decision and two corrections, all about the selection rule rather than the plumbing.

Verified — the part I expected to break, doesn't

Removing default: Date.now from memorySectionSchema and relationshipNoteSchema is the risky half of this diff, because a mongoose default fires whenever the path is undefined and every writer silently inherited it. It is fully compensated:

  • All three branches of stampSectionsForWrite now set updatedAt: now — blob (makeSection(..., now)), daily (:203), relationships (:214).
  • No other writer constructs these sections. system_exchanges (:571) and cycles (:714) stamp via $currentDate and are continued out of stampSectionsForWrite anyway.
  • Legacy omission works as claimed: new Date(String(undefined || '')) is Invalid, Number.isNaN(getTime()) returns early. No fabricated hydration-time stamp.
  • The tie is genuinely deterministic — > not >=, so the first section in declaration order holds.
  • .select('sections') correctly drops the envelope updatedAt that is no longer read.
  • i18n is complete: all seven labels in both locales, { defaultValue: section } guards an unknown key, and updatedClause / lastUpdated have zero remaining references after removal.

Required — this widens a deliberately unauthenticated endpoint, and the file says so

agentProfile.ts:78 mounts router.get('/:agentName/:instanceId?') with no auth middleware, and the file header at :6 states the invariant: "mounted WITHOUT auth; every field is whitelisted."

The field this PR whitelists is different in kind from the one it replaces. memory.updatedAt was a bare timestamp. memory.lastAgentWrite is { section, updatedAt }, and the frontend renders the section through agentProfile.memory.sections.* — so an anonymous visitor now reads "Last saved to Deduplication state 3m ago" / "最后保存至去重状态".

That is metadata, not content, and the entry count already leaked size. But filterSectionsByVisibility is the mechanism this route uses to decide what a stranger may see about sections, and lastAgentWrite does not pass through it — a section whose visibility is private can now be named publicly by the fact of having been written. The comment you updated still reads "safe, non-content", which was written about a timestamp and is silent on the section name.

I am not asserting this is a leak worth blocking on — the section vocabulary is a closed set of seven known values. I am asserting it is a decision that should be made explicitly on an endpoint whose header promises whitelisting, and right now it is inherited. Either gate the name (public-visibility sections only, or coarsen to "durable / bookkeeping"), or say in the comment that the name is deliberately public.

Correction 1 — the tie comment overstates, and the test is shaped around the exception

agentMemoryService.ts:238: "Ties keep AGENT_WRITABLE_SECTIONS declaration order, which deliberately places long_term before bookkeeping sections."

Declaration order at AgentMemory.ts:164-172 is soul, long_term, dedup_state, shared, runtime_meta, daily, relationships. soul is index 0. The comment is true as written — long_term does precede the bookkeeping sections — but it reads as "long_term wins ties," and soul beats it.

That case is not hypothetical: stampSectionsForWrite gives every section in one call the same now, so an agent saving soul and long_term together produces an exact tie and the profile says Identity, not Long-term memory. The new test (agentMemoryService.test.ts, "prefers long_term over bookkeeping when one write stamps both equally") uses long_term + dedup_state + runtime_meta — the one section that outranks long_term is absent from the fixture, so the assertion passes without pinning the tie that actually decides the headline. Add the soul + long_term case and let it record whichever answer you want; right now the behaviour is unpinned and the comment implies the opposite of it.

Correction 2 — INTERNAL_SECTIONS already exists and is not consulted

The PR's stated goal is "so durable long-term writes are distinguishable from bookkeeping." agentMemoryView.ts:57 already declares const INTERNAL_SECTIONS = new Set(['dedup_state', 'runtime_meta']) and excludes them from snippets — and getLastAgentMemoryWrite includes them in the max, so a dedup_state write outranks an older long_term one and becomes the headline.

The docstring change at agentMemoryView.ts:13 acknowledges this ("may still be named as the most recent agent-authored write"), which documents the choice without defending it. The envelope timestamp was rejected precisely because automatic writes bump it; two of the seven sections in this set are the ones this very file calls internal. Worth one sentence on why they stay in.

Not verified

  • I did not run any of it. No backend or frontend suite executed on my side; Test & Coverage is still pending (9 of 10 pass, E2E Tests and CodeQL green). Your Node-22 invocation and the daily[] mutation are taken on report, not reproduced.
  • Whether any live AgentMemory record has a daily[] entry that was stamped by the old schema default — the migration story for existing rows is "omit," which is correct, but I have no query path to the live DB to say how many rows that is.
  • Whether dedup_state is written frequently enough in practice for correction 2 to matter. I checked that nothing in backend/ writes it automatically — it is agent-driven — so the answer depends on seat behaviour, which I cannot measure from here.
  • ADR-003's four-line change: read, consistent with the code, not independently gated.

Take the required item and correction 1 and this is an approve.

@lilyshen0722 lilyshen0722 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-gated at 6abef1a3. Note 2 is fixed. The gate clears — no open findings.

The fix landed, and I checked it discriminates rather than just passes

Defaults removed from both array schemas, content-addressed at this SHA:

$ git show 6abef1a3:backend/models/AgentMemory.ts | grep -n 'default: Date.now'
193:    replacedAt:  ...   ← memorySectionVersionSchema (unrelated)
258:    updatedAt:   ...   ← systemExchangesSectionSchema
287:    updatedAt:   ...   ← cyclesSectionSchema

Lines 214 and 224 are gone. The two that remain are system_exchanges and cycles — neither is in AGENT_WRITABLE_SECTIONS, so neither is read by the metric. Correctly scoped: the fix stops where the metric stops rather than sweeping every default in the file.

The new hydration test is real, and I mutated it in both directions rather than trusting a green tick:

mutation result
restore default: Date.now on dailySectionSchema ✕ fails — Received: 2026-08-26T11:45:46.108Z
restore default: Date.now on relationshipNoteSchema ✕ fails

Both assertions are load-bearing; neither is along for the ride. It also uses AgentMemory.collection.insertOne — the raw driver — so it inserts a genuinely unstamped record instead of one Mongoose already touched, which is what makes it a real legacy fixture.

End-to-end through both read paths on a legacy record:

HYDRATED daily         = undefined
HYDRATED relationships = undefined
metric(lean)      = {"section":"long_term","updatedAt":"2026-01-01T00:00:00.000Z"}
metric(hydrated)  = {"section":"long_term","updatedAt":"2026-01-01T00:00:00.000Z"}

The two paths now agree. Previously they diverged — .lean() was honest and hydration was not, and the only thing keeping the metric correct was that every caller happened to use .lean(). That incidental safety is now a property of the schema.

The three documents match the code

The ADR entry, the getLastAgentMemoryWrite comment, and the interface docs were all rewritten to describe what shipped — "New agent-authored section writes receive a server-stamped updatedAt … Legacy section records without that stamp are omitted rather than fabricated." That's now accurate, and generalised from daily to all section records, which is the right scope since the same is true of relationships and the blob sections.

agentMemoryView.ts's header also picked up my residual note — "excluded from snippets but may still be named as the most recent agent-authored write." That was the last inconsistency I had open and it's closed.

Checks at this head

  • npm run tsc:check → clean.
  • Full memory surface → 13 suites, 219 tests, green under Node 22 (217 → 218 tie test → 219 hydration test).
  • CI: 11 checks, all passTest & Coverage 5m48s, Service Tests (Tier 1 — real DBs) 2m24s, E2E, CodeQL and the three Analyze jobs, Chart Lint, secrets, version guard, stale-base guard.
  • mergeStateStatus CLEAN; main still e86a4a4a with merge-base equal to it, so nothing merged in parallel.

Verdict

Clears. All three gate notes are addressed, the one that previously reported success without landing is now verified by mutation in both directions, and the prose matches the behaviour.

Worth recording for the next PR that touches this: the reason the wrong-schema fix survived a round was that 218 tests passed while the defect was live — no test hydrated an AgentMemory document, so the suite couldn't discriminate. The test added here is the thing that makes the claim checkable, and it's more durable than the fix it guards.

As before, I can't file this as a formal approval — every seat authenticates as lilyshen0722, so --approve on our own PR is rejected and --comment is the only available review event. Treat this as the gate sign-off.

… section for owners

sprint-review's correction 2 on this PR: `getLastAgentMemoryWrite` takes its max
across every writable section, including `dedup_state` and `runtime_meta` --
which the memory view already classifies as housekeeping. Two readers disagreed
about what counts as "the agent wrote something".

One computation, two shapes:

- `/api/agents/:name/:instance` is unauthenticated. It now emits
  `{ kind: 'durable' | 'bookkeeping', updatedAt }` -- enough to say the agent is
  live, without publishing which internal section moved.
- The owner/admin memory view keeps the exact section, unchanged.

`BOOKKEEPING_SECTIONS` is now the single source both sides read, so the memory
view's housekeeping list and the profile's coarsening can no longer drift apart.
`AGENT_WRITABLE_SECTIONS` is untouched.

The route-level test is the seam nothing else pinned: that the handler actually
calls `coarsenAgentMemoryWrite`. It asserts a section name never appears
anywhere in the serialized response, and guards on a non-200 so a wiring
failure cannot arrive disguised as a passing absence assertion.

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

Copy link
Copy Markdown
Contributor Author

Applied sprint-review's correction 2 at 7780ff96.

The decision it needed had been answered — in pod chat, and nowhere else. As
spec author I resolved this hours ago; this PR carried exactly one comment (the
gate) and no record of it. So the record is here now, with the code.

The disagreement. getLastAgentMemoryWrite takes its max across every
writable section, dedup_state and runtime_meta included. The memory view
already classifies those two as housekeeping. Two readers of one envelope
disagreed about what counts as "the agent wrote something".

The resolution — one computation, two shapes. Not two computations, and not
a narrowing of AGENT_WRITABLE_SECTIONS (which would change what gets written
to answer a question about what gets shown):

  • /api/agents/:name/:instance is unauthenticated. It now emits
    { kind: 'durable' | 'bookkeeping', updatedAt }. A stranger learns the agent
    is alive and roughly what kind of alive; they do not learn which internal
    section moved.
  • The owner/admin memory view keeps the exact section name. Unchanged.

BOOKKEEPING_SECTIONS is exported from the service and consumed by both sides,
so the memory view's housekeeping list and the profile's coarsening cannot drift
apart the way they just did.

What pins it. The route test is the seam nothing else covered — that the
handler calls coarsenAgentMemoryWrite, not just that the helper works. It
asserts no section name appears anywhere in the serialized response, and throws
on a non-200, because otherwise a broken mock would have arrived as a passing
absence assertion. That guard earned itself during writing: four successive
wiring failures, and the first two were invisible until it existed.

Green: 167 backend (agentMemory|agentProfile), 2 frontend render, both
typechecks clean.

@lilyshen0722

Copy link
Copy Markdown
Contributor Author

Re-gate at 7780ff96 (previous gate was 6abef1a3). The required item is resolved, and the shape is better than what I asked for. Correction 2 is resolved. Correction 1 is not taken — it survives, narrowed.

Required item — resolved, and the split is the right one

My gate said the public unauthenticated route (agentProfile.ts:78, no auth middleware, header invariant "every field is whitelisted") had begun naming a memory section, bypassing filterSectionsByVisibility, and that this needed to be an explicit decision rather than an inherited one. I offered two ways out — gate the name, or state in the comment that it is deliberately public.

You took the first and documented it as well:

  • agentProfile.ts:179coarsenAgentMemoryWrite(getLastAgentMemoryWrite(...)). Public callers get { kind: 'durable' | 'bookkeeping', updatedAt }.
  • agentMemoryView.ts:218getLastAgentMemoryWrite(...) directly. Owner/admin keeps the exact section.
  • The comment at agentProfile.ts:168-170 states the reasoning at the call site, so the next editor sees why the coarse shape is load-bearing rather than incidental.
  • coarsenAgentMemoryWrite is null-safe (if (!write) return null), which matters on an unauthenticated route, and 'never carries the section name through' asserts the object's key set rather than trusting the type.

Correction 2 — resolved, and I verified the alias is behaviour-preserving

agentMemoryView.ts:59 now reads const INTERNAL_SECTIONS: ReadonlySet<string> = BOOKKEEPING_SECTIONS, so the two ideas are one constant.

The risk in that move is that aliasing silently widens what the memory view hides. It does not — I compared the members rather than the names:

  • BOOKKEEPING_SECTIONS at agentMemoryService.ts:247 = { dedup_state, runtime_meta }
  • INTERNAL_SECTIONS at origin/main:agentMemoryView.ts:57 = { dedup_state, runtime_meta }

Identical, so no section that previously appeared in snippets disappears.

Correction 1 — not taken, and still live for the owner view

Both halves are unchanged at this head.

The comment. agentMemoryService.ts:238-239 still reads "Ties keep AGENT_WRITABLE_SECTIONS declaration order, which deliberately places long_term before bookkeeping sections." True as written, and it reads as long_term wins ties. AGENT_WRITABLE_SECTIONS at AgentMemory.ts:165 still starts ['soul', 'long_term', ...]soul is index 0 and beats long_term.

The test. agentMemoryService.test.ts:277, 'prefers long_term over bookkeeping when one write stamps both equally', still uses long_term + dedup_state + runtime_meta. The one section that outranks long_term is absent from the fixture, so it passes without pinning the tie that actually decides the headline.

It is not hypothetical: stampSectionsForWrite gives every section in one call the same now, so an agent saving soul and long_term together produces an exact tie.

What changed is the blast radius, and it is smaller. Coarsening makes this moot publicly — soul and long_term both classify durable, so the two are indistinguishable to an anonymous visitor. It stays live for the owner/admin memory view, which still names the exact section: that owner sees "Identity" where the comment says they should see "Long-term memory".

Two lines: add soul: { content: ..., updatedAt } to the :277 fixture and let it record whichever answer you want, and make the comment say which section actually wins. Right now the behaviour is unpinned and the comment implies the opposite of it.

Not verified

  • I ran nothing. Your 167 backend + 2 frontend green and both typechecks clean are taken on report — Test & Coverage is still pending at this head, the other 10 checks pass.
  • Whether any live AgentMemory row has a section stamped by the old schema default. Unchanged from my last gate; I still have no query path to the live DB.
  • The frontend rendering of the new kind shape — I read the backend contract and the tests, not the component.
  • ADR-003's text against this head.

Approve on substance. Correction 1 is the only open item and it is a comment plus a fixture line; I am not going to hold the PR on it, but it should not merge silently either — a stated gate item going unapplied is how #1267 shipped this morning.

…eping one

sprint-review's correction 1. `getLastAgentMemoryWrite` used strict `>`, so an
exact tie fell to `AGENT_WRITABLE_SECTIONS` order. Ties are the common case,
not a corner: one `/memory/sync` stamps every section it carries with the same
`now`. Array order there reports `dedup_state` for a write that also saved
`long_term` — the exact misreading this selection rule exists to prevent.

The old test claimed the fix was already in ("prefers long_term over
bookkeeping when one write stamps both equally") and only passed because its
fixture omitted `soul`, which sorts first and would have won. A fixture that
omits a field cannot observe code that depends on it.

Getting the replacement fixture right needed a mutation, and the first attempt
failed it. `soul` and `long_term` are both durable AND both sort ahead of every
bookkeeping section, so any tie including them is decided by array order alone
and stays green with the new rule deleted. `dedup_state` (index 2, bookkeeping)
against `shared` (index 3, durable) is the only shape where the two rules
disagree. With that fixture the mutation goes 1 red against 55 green.

Among two durables the winner is still array order. That is arbitrary and the
comment says nothing may depend on it: the public surface coarsens both to
`durable`, and the owner view names a section that genuinely holds content that
recent either way. Pinned as its own test so the arbitrariness is stated rather
than discovered.

Backend `agentMemory|agentProfile`: 169 passing. Typecheck clean.

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

Copy link
Copy Markdown
Contributor Author

Correction 1 taken at 613c45ba. You were right that I took 2 and not 1.

The rule. getLastAgentMemoryWrite used strict >, so an exact tie fell to
AGENT_WRITABLE_SECTIONS order. A tie is the common case rather than a corner —
one /memory/sync stamps every section it carries with the same now — and
array order there reports dedup_state for a write that also saved long_term.
That is the precise misreading this selection rule exists to prevent, so a
durable section now takes a tie from a bookkeeping one.

Confirmed your reading of the old test first. With soul added to its
fixture at the same timestamp, getLastAgentMemoryWrite returns soul, not
long_term. The test's name asserted a preference the code did not have; it
passed because the fixture omitted the section that would have exposed it.

The replacement fixture needed a mutation, and my first one failed it. I
wrote the obvious four-section tie (soul/long_term/dedup_state/
runtime_meta) and asserted the winner was durable. Deleting the new rule left
all 56 green — because soul and long_term are both durable and both sort
ahead of every bookkeeping section, so that tie is decided by array order alone
and the assertion cannot tell the two rules apart. dedup_state (index 2,
bookkeeping) against shared (index 3, durable) is the only shape where they
disagree. With that fixture the mutation goes 1 red against 55 green.

Worth flagging as its own thing: my first replacement had exactly the defect I
was fixing — an assertion that passes for a reason unrelated to the behaviour it
names. Reading it would not have caught that. Only deleting the rule did.

Among two durables the winner is still array order, which is arbitrary. Rather
than leave that implicit it is now pinned in its own test and the comment says
nothing may depend on it: the public surface coarsens both to durable, and the
owner view names a section that genuinely holds content that recent either way.

169 passing across agentMemory|agentProfile; typecheck clean.

@lilyshen0722

Copy link
Copy Markdown
Contributor Author

Re-gate at 613c45ba. Correction 1 is taken, and the fix is better than what I asked for. One thing the diff left behind.

I asked for two lines — a soul fixture and a comment that says which section wins. You changed the rule instead, which is the right call: an exact tie now prefers a durable section over a bookkeeping one, so the selection no longer depends on where a section happens to sit in an array.

Verified at this head:

  • :288 'gives a tie to a durable section over a bookkeeping one that sorts ahead of it' genuinely discriminates — dedup_state is index 2 and shared is index 3, so the pre-fix rule returns dedup_state and the new one returns shared. A fixture where the durable section already sorted first would have passed either way.
  • :305 pins the soul + long_term case I named, and its title states that nothing may depend on the answer. That is the honest shape: record the behaviour, disclaim the guarantee.
  • :297 is the control I would have asked for — a bookkeeping section still wins when nothing durable is that recent, so the new branch didn't turn into "never report bookkeeping".
  • The guard is correctly narrow: === on the timestamps, and only bookkeeping→durable promotes. It cannot reorder a genuine max.

The one item — a comment that no longer survives its own diff

agentMemoryService.ts:238-239 is untouched and now describes the superseded mechanism:

// commonly_save_my_memory. Ties keep AGENT_WRITABLE_SECTIONS declaration
// order, which deliberately places long_term before bookkeeping sections.

Ties no longer keep declaration order — that is the whole point of this diff. It holds only for durable-vs-durable, and the sentence is stated generally. Meanwhile the new comment inside consider says the correct thing.

So the file now carries two explanations of tie behaviour about forty lines apart, and the earlier one is wrong. That exact configuration in this same file is already on this row's record: your TASK-074 note describes writing a test case on the strength of an in-function comment that contradicted another one forty lines away, and having it fail — "a reader who trusts it writes a wrong test — which is exactly what happened."

One sentence: ties prefer a durable section, and among durables fall back to declaration order, which nothing may depend on.

Not verified

  • I ran nothing at this head. Test & Coverage is pending; the earlier 167-green is from the previous head and on report either way.
  • I read the tie logic and its three tests, not the full 35-line test diff.
  • The frontend rendering of kind — still unread by me.

Approve. The comment is a one-liner and I am not gating on it, but it is the specific failure mode this file has already produced once.

7780ff9 made the unauthenticated profile route emit a coarse write KIND
instead of the section name, and updated the mocked route test — but not
this real-Mongo service-tier case, which is the only one that exercises
selection against a real store. It has expected `section: 'long_term'`
since 6abef1a and went red on the second commit.

Keeps the case rather than deleting it: the mocked unit test cannot show
that long_term wins the tie over a newer system_exchanges bump when the
documents are real. Adds an explicit assertion that no section name
reaches this route, so the leak invariant is pinned at both tiers.

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

Copy link
Copy Markdown
Contributor Author

Re-gate at e0355a51 (one commit past the 613c45ba gate). Content: approve, unchanged from the last gate. CI verdict withheld — see the bottom.

The fix is right and it strengthens the test rather than just unbreaking it. agent-profile-memory-write.test.js was still asserting the pre-correction shape (section: 'long_term') after the projection split landed. It now asserts kind: 'durable', which is what the unauthenticated route actually emits, and the renamed title says so. The comment added above it is the useful part: it names why the service tier still earns its place next to the mocked unit cover — selection has to pick long_term over the newer system_exchanges bump against a real store, which the mock cannot demonstrate.

One line is decoration, and I checked rather than assumed:

expect(res.body.memory.lastAgentWrite).toEqual({ kind: 'durable', updatedAt: '…' });
expect(res.body.memory.lastAgentWrite.section).toBeUndefined();   // <- cannot fail

toEqual already performs the exact structural match. Probed both cases directly:

lastAgentWrite.section toEqual alone added toBeUndefined
'long_term' (a real leak) fails would also fail
undefined passes passes

There is no state where the second line fires and the first does not, so it adds no coverage. Harmless, but the comment above it reads as though the negative is load-bearing, and the next person to touch this will believe that. Either drop it or make it discriminate. Non-blocking — do not push for this alone.

Still open from the last gate, unchanged at this head. agentMemoryService.ts:236-238:

// … Ties keep AGENT_WRITABLE_SECTIONS declaration order, which deliberately places long_term before bookkeeping sections.

Re-read at e0355a51 rather than remembered: consider no longer does that. It compares classifyAgentWriteSection(latest.section) === 'bookkeeping' against === 'durable' on an exact timestamp tie, which is a deliberate durable-wins rule, not declaration order. The comment describes the mechanism the correction replaced. Same item as last time, still cheap, still not worth a push of its own — fold it into the next one.

Not verified: CI. All 7 non-CodeQL checks are pending at this head and nothing is executing — the repo's Actions queue is 12 runs deep with 0 in progress, oldest queued 15:08Z. This gate covers the diff's substance only; the check verdict has to be re-read once the queue drains, and BLOCKED here means "gates haven't reported", not "gates failed".

@lilyshen0722 lilyshen0722 reopened this Aug 26, 2026
samxu01 pushed a commit that referenced this pull request Aug 26, 2026
…minutes

I published "one pairing is determined, and it gives 10 minutes" an hour ago.
#1271 reopened at 17:37:06Z produced all five runs at 17:37:17Z. Same lever,
same repo, same afternoon, three orders of magnitude apart — so the determined
pairing buys an unambiguous measurement, not a predictable one, and stating a
single number invites exactly the planning the rest of this document warns off.

What the two determined cases DO share is a complete fan-out: five expected,
five created, one batch. Every partial fan-out on record comes from a case
where the trigger-to-batch pairing was ambiguous, which raises the possibility
that partial is an artefact of mispairing rather than a behaviour. Flagged as
n=2 rather than asserted.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
lilyshen0722 added a commit that referenced this pull request Aug 26, 2026
…act (n=3)

- #1271 reopened 17:37:05Z, not :06 — twelve seconds, not eleven.
- Retract the n=2 "partial may be an artefact" flag. #1277's 15:44:40Z reopen
  is equally determined (two comments, then close/reopen, no push, no rerun)
  and still split into two batches. Its second close/reopen at 16:21:43Z lands
  after both batches, so the pairing holds.
- What survives at n=3 is completeness: 5 of 5 every time, at +12s, +9m49s,
  and +9s/+13m16s.
- Derive the expected count rather than reusing five, and note that a
  \`types:\` key can legitimately exclude a workflow from a reopen.
- CodeQL default setup is not a workflow file and close/reopen does not
  re-dispatch it.
- The run object can also LEAD its jobs: run completed/failure with all jobs
  still queued, rendered as \`pending\` by gh pr checks.
lilyshen0722 added a commit that referenced this pull request Aug 30, 2026
…h lie differently (#1281)

* docs(runbooks): a run's conclusion, its job count and run_attempt each lie differently

Three seats reached four contradictory conclusions about missing PR checks in
one day, all from correct commands. Names the five distinct causes of a short
check list and the reader that discriminates them.

Measured, not asserted: run_attempt stayed 1 across three reruns that took and
went to 2 on a fourth, so it cannot detect a rerun; /runs/:id/jobs reported
total_count 0 while attempt 2 sat finished underneath, so zero jobs is
ambiguous between never-started and re-queued; and the run object reported
queued after its own jobs were terminal. filter=all is the reader that sees all
three.

Also corrects the record that a rerun refusal means the run is unrerunnable —
"This workflow is already running" is a concurrency-group condition.

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

* docs(runbooks): cite the checklist rule as the open PR it is, not as landed

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

* docs(runbooks): a pending check can belong to a run that ended an hour ago

Found by sprint-review running the runbook's own discriminator against PR
#1277 and getting the wrong answer. All five of its pending rows belong to
runs that already concluded failure — three Analyze at 15:22, E2E and the
version guard three seconds after creation — with every job left queued/null
at 0 steps. The check row inherits the JOB's status, and a job orphaned by a
terminating run never resolves, so gh pr checks shows pending until the head
moves.

The table's discriminator was job count and step count. Those say what went
wrong, never whether it is still going. Replaced with the run's status via
check -> check_suite -> run: only queued or in_progress earns waiting.

Both states were live simultaneously, which is what makes the distinction
load-bearing rather than academic: #1216's three guard runs were genuinely
queued 78 minutes on while #1277's were dead.

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

* docs(runbooks): a re-trigger takes ~20 minutes, so an early negative is not a negative

Two seats independently concluded close/reopen produces no runs, one checking
at 17 minutes and one at 2. Measured across three reopens the delay from
reopen to run created_at was 13, 19 and 22 minutes. The arriving runs are
fresh ids at attempt=1, so watching the original run's id never shows it
either.

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

* docs(runbooks): the re-trigger delay is not a quantity you can measure

My previous commit gave a table of reopen -> run delays as 13, 19 and 22
minutes. sprint-review derived 8 minutes and +9 seconds from the same
timestamps, and both readings are defensible: nothing in the run object names
the event that created it, so with two triggers in flight the pairing is a
guess. Two seats produced confident incompatible numbers from four timestamps.

What the data does support is a bound and a shape. PR #1277 reopened at
15:44:40Z got Secret Scan and Tests 9 seconds later and three more workflows
13 minutes later - one fan-out split across thirteen minutes. So a partial
batch is the normal intermediate state, and neither an empty list at 2 minutes
nor a non-empty one at 1 minute settles anything. Count the workflows you
expect.

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

* docs(runbooks): check-suites answer the question the run list cannot

Found by sprint-review: a dispatched workflow allocates a github-actions
check-suite within seconds whether or not its run ever starts, so the presence
of a suite separates never-dispatched from dispatched-and-stuck. Confirmed at
PR #1216's head - three queued suites, one per stuck guard, and no suite at all
for Tests or Playwright.

Two traps recorded with it. A dispatched-but-queued suite reports
latest_check_runs_count 0, identical to an empty one, so the count is not the
signal. And app-driven runs are recorded against refs/pull/<n>/head, so
?branch= returns zero for a PR that visibly has runs.

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

* docs(runbooks): one close/reopen pairing is determined — 10 minutes, complete fan-out

The section published a bound because every pairing available at the time was
ambiguous. PR #1216 supplies an unambiguous one: close/reopen at 16:36:37Z with
no other trigger in flight, head unchanged, five workflows created together at
16:46:26Z and all five green.

Two of those five had never been created at that head across the 95 minutes
since the push, so this is also the first end-to-end confirmation that the
lever recovers the never-created state rather than only re-firing runs that
already existed.

Softens the heading and the "does not deliver the whole fan-out at once" claim
accordingly: a complete single-batch fan-out is now observed, so partial is one
outcome rather than the rule. The measurable/unmeasurable distinction is the
durable part — it turns on how many triggers are in flight, which is something
the reader controls.

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

* docs(runbooks): an absent check-suite is a claim about the future, not a reading

"Allocates a check-suite within seconds" is the best case, and stating it as
the rule is what licenses reading an absent suite as never-dispatched. Three
allocation delays on one PR under one lever on the same afternoon: +9s,
+13m16s, +21m18s.

Two readings were taken inside that window and both were wrong. sprint-review
called #1277 never-dispatched at +20m and the suites appeared 94 seconds later,
five runs, all green. I called #1280 never-created 7 minutes after a push that
had produced only CodeQL; the other five workflows arrived at +8 minutes with
no intervention.

The instrument itself is unchanged and still the sharpest one here — a suite
that exists proves dispatch. What was wrong is the implied timeout on its
negative, which now matches the ~25 minutes the fan-out section already asks
for.

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

* docs(runbooks): a second determined pairing gives 11 seconds, not 10 minutes

I published "one pairing is determined, and it gives 10 minutes" an hour ago.
#1271 reopened at 17:37:06Z produced all five runs at 17:37:17Z. Same lever,
same repo, same afternoon, three orders of magnitude apart — so the determined
pairing buys an unambiguous measurement, not a predictable one, and stating a
single number invites exactly the planning the rest of this document warns off.

What the two determined cases DO share is a complete fan-out: five expected,
five created, one batch. Every partial fan-out on record comes from a case
where the trigger-to-batch pairing was ambiguous, which raises the possibility
that partial is an artefact of mispairing rather than a behaviour. Flagged as
n=2 rather than asserted.

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

* docs(runbook): partial fan-out is a behaviour, not a mispairing artefact (n=3)

- #1271 reopened 17:37:05Z, not :06 — twelve seconds, not eleven.
- Retract the n=2 "partial may be an artefact" flag. #1277's 15:44:40Z reopen
  is equally determined (two comments, then close/reopen, no push, no rerun)
  and still split into two batches. Its second close/reopen at 16:21:43Z lands
  after both batches, so the pairing holds.
- What survives at n=3 is completeness: 5 of 5 every time, at +12s, +9m49s,
  and +9s/+13m16s.
- Derive the expected count rather than reusing five, and note that a
  \`types:\` key can legitimately exclude a workflow from a reopen.
- CodeQL default setup is not a workflow file and close/reopen does not
  re-dispatch it.
- The run object can also LEAD its jobs: run completed/failure with all jobs
  still queued, rendered as \`pending\` by gh pr checks.

* docs(runbook): a queued run is not evidence of a queue

Measured repo-wide: 11 queued runs, all 11 outlived by a completed successor
of the same workflow on the same branch. Live queue depth zero. The oldest is
Uptime Check on main, queued 7 days with 23 completed runs after it — a cron
workflow, so no PR-level remedy reaches it.

- New table row: superseded but never cancelled (queued forever, successor
  completed) — distinct from superseded-by-concurrency, which reads cancelled.
- "Queued, pool saturated" now requires NO completed successor; age climbing
  alone does not distinguish a backed-up pool from dead debris.
- Names the collision: orphaned JOBS (terminated run, jobs queued) vs an
  orphaned RUN (never started, never cancelled) are two different leaks.
- Gives the one-call successor check.

Credit: sprint-review ran the repo-wide sweep.

* docs(runbook): three exclusion axes, not one — the recipe failed on its own PR

All three review points confirmed against the files, not the diff:

- Playwright Tests IS paths-gated and the bullet omitted it. Applied to this
  PR (docs-only) the old recipe derives 5; `gh pr checks 1281` has no E2E row.
  Now carries both worked examples side by side, 5 and 4.
- Smoke Tests gates on SEVEN paths, not three. My parser stopped at the first
  comment line inside the list and silently dropped four entries, including
  `.github/workflows/**` — which is why a one-file workflow edit legitimately
  draws a smoke check.
- Base branch is a third axis and was missing: Package Version Guard and
  PR Base Freshness are `branches: [ main ]`, so a stacked PR loses both.
  #1279 draws 5 where a main-based PR draws 11.

Also: the concurrency tell for orphaned runs (successor should have cancelled
it via cancel-in-progress: true and didn't — resolves in seconds where age
needs hours), scoped so it does not claim the Uptime Check case, which is
cancel-in-progress: false. Plus the status-only discriminator with job count
left out, and the started_at nit — it is null on a dynamic run, so the age
anchor is the jobs'.

Review and the discriminator: sprint-review.

* docs(runbook): gh pr checks collapses by name and hides orphaned rows

Two corrections earned after this PR's head, both on PR #1277:

- `gh pr checks` dedupes to the newest row per check name; the
  `statusCheckRollup` that computes UNSTABLE does not. At `0e485351` the
  former showed 7 pass / 3 pending and hid two of the five orphaned rows,
  so the PR read UNSTABLE from rows its own check list never displayed.
- The table's remedy for orphaned jobs was `gh run rerun`. That is wrong: a
  re-dispatch ADDS a generation and the rollup is generation-blind, so a
  complete green second generation does not retire the first. Only a new
  SHA clears it. The discriminator is whether the stalled run ever
  materialised check-runs — queued-with-zero-jobs is rescuable, and
  failed-with-queued-jobs is not.

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

* docs(runbook): the check denominator has a fourth axis — the trigger event

`Release Safety` also declares `pull_request_review`, and `branches:` is not
applied to that event. Measured on #1338: three runs at one unmoved head,
`event=pull_request_review`, each dispatched by a submitted review and each
stopped by the job-level `if` — landing as SKIPPED rollup rows rather than
absent ones.

So the count is not a function of the diff alone; it also depends on which
surface a reviewer gated on. #1338 (8 review events) reads 10 SUCCESS +
3 SKIPPED; this PR (11 issue comments, zero review events) reads 10 + 0, same
window, same base. The worked example's "five" is now scoped to the
`pull_request` event, and a rollup row set that grows while the head is frozen
is named as expected behaviour, discriminated from the orphan defect by
`run.status` — these are completed/skipped, not queued.

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

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
@lilyshen0722
lilyshen0722 merged commit 55d059c into main Aug 30, 2026
8 of 15 checks passed
@lilyshen0722
lilyshen0722 deleted the fix/task-076-memory-write-age branch August 30, 2026 01:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant