Recover archived (.reset) session transcripts in memory hook + session-logs skill#71537
Recover archived (.reset) session transcripts in memory hook + session-logs skill#71537injinj wants to merge 3 commits into
Conversation
Greptile SummaryThis PR fixes two surfaces that silently ignored archived ( Confidence Score: 4/5Safe to merge; only P2 documentation style concerns remain. No P0 or P1 issues found. The two P2 findings are confined to the SKILL.md documentation snippet: skills/session-logs/SKILL.md — minor bash hygiene in the helper snippet. Prompt To Fix All With AIThis is a comment left during a code review.
Path: skills/session-logs/SKILL.md
Line: 73-80
Comment:
**`shopt -s nullglob` set at global scope modifies caller's shell environment**
Setting `nullglob` outside the function makes the helper non-self-contained — any script that sources or copy-pastes this snippet gets its glob behavior silently changed for all subsequent code. Moving the option inside the function with a save/restore is more robust:
```bash
list_session_transcripts() {
local _nullglob_was_set
_nullglob_was_set=$(shopt -p nullglob 2>/dev/null)
shopt -s nullglob
for f in "$SESSION_DIR"/*.jsonl \
"$SESSION_DIR"/*.jsonl.reset.*Z \
"$SESSION_DIR"/*.jsonl.deleted.*Z; do
[ -f "$f" ] && printf '%s\n' "$f"
done
eval "$_nullglob_was_set"
}
```
Minor as this is doc code, but the current placement will surprise anyone who sources it from a larger script.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: skills/session-logs/SKILL.md
Line: 103-105
Comment:
**`for f in $(...)` is word-split-unsafe for paths with spaces**
`for f in $(list_session_transcripts)` will break if any session path contains spaces (or other IFS characters). The safer pattern is a `while IFS= read -r` loop:
```bash
while IFS= read -r f; do
# process "$f"
done < <(list_session_transcripts)
```
This is a documentation snippet so impact is limited, but copying it verbatim into a script on a system with unusual usernames or paths would silently truncate or misparse file names.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "docs(session-logs): include archived tra..." | Re-trigger Greptile |
| shopt -s nullglob | ||
| list_session_transcripts() { | ||
| for f in "$SESSION_DIR"/*.jsonl \ | ||
| "$SESSION_DIR"/*.jsonl.reset.*Z \ | ||
| "$SESSION_DIR"/*.jsonl.deleted.*Z; do | ||
| [ -f "$f" ] && printf '%s\n' "$f" | ||
| done | ||
| } |
There was a problem hiding this comment.
shopt -s nullglob set at global scope modifies caller's shell environment
Setting nullglob outside the function makes the helper non-self-contained — any script that sources or copy-pastes this snippet gets its glob behavior silently changed for all subsequent code. Moving the option inside the function with a save/restore is more robust:
list_session_transcripts() {
local _nullglob_was_set
_nullglob_was_set=$(shopt -p nullglob 2>/dev/null)
shopt -s nullglob
for f in "$SESSION_DIR"/*.jsonl \
"$SESSION_DIR"/*.jsonl.reset.*Z \
"$SESSION_DIR"/*.jsonl.deleted.*Z; do
[ -f "$f" ] && printf '%s\n' "$f"
done
eval "$_nullglob_was_set"
}Minor as this is doc code, but the current placement will surprise anyone who sources it from a larger script.
Prompt To Fix With AI
This is a comment left during a code review.
Path: skills/session-logs/SKILL.md
Line: 73-80
Comment:
**`shopt -s nullglob` set at global scope modifies caller's shell environment**
Setting `nullglob` outside the function makes the helper non-self-contained — any script that sources or copy-pastes this snippet gets its glob behavior silently changed for all subsequent code. Moving the option inside the function with a save/restore is more robust:
```bash
list_session_transcripts() {
local _nullglob_was_set
_nullglob_was_set=$(shopt -p nullglob 2>/dev/null)
shopt -s nullglob
for f in "$SESSION_DIR"/*.jsonl \
"$SESSION_DIR"/*.jsonl.reset.*Z \
"$SESSION_DIR"/*.jsonl.deleted.*Z; do
[ -f "$f" ] && printf '%s\n' "$f"
done
eval "$_nullglob_was_set"
}
```
Minor as this is doc code, but the current placement will surprise anyone who sources it from a larger script.
How can I resolve this? If you propose a fix, please make it concise.| _Tip:_ swap the `for f in ...` line for `for f in $(list_session_transcripts); do` | ||
| (see snippet above) when you also want archived `.reset` / `.deleted` files in the | ||
| listing. |
There was a problem hiding this comment.
for f in $(...) is word-split-unsafe for paths with spaces
for f in $(list_session_transcripts) will break if any session path contains spaces (or other IFS characters). The safer pattern is a while IFS= read -r loop:
while IFS= read -r f; do
# process "$f"
done < <(list_session_transcripts)This is a documentation snippet so impact is limited, but copying it verbatim into a script on a system with unusual usernames or paths would silently truncate or misparse file names.
Prompt To Fix With AI
This is a comment left during a code review.
Path: skills/session-logs/SKILL.md
Line: 103-105
Comment:
**`for f in $(...)` is word-split-unsafe for paths with spaces**
`for f in $(list_session_transcripts)` will break if any session path contains spaces (or other IFS characters). The safer pattern is a `while IFS= read -r` loop:
```bash
while IFS= read -r f; do
# process "$f"
done < <(list_session_transcripts)
```
This is a documentation snippet so impact is limited, but copying it verbatim into a script on a system with unusual usernames or paths would silently truncate or misparse file names.
How can I resolve this? If you propose a fix, please make it concise.|
Cross-reference: this PR fixes the auto-summarize-on-reset hook + the The two PRs touch entirely disjoint files:
Verified with a local merge of both branches onto
Combined branch on the author's fork for reviewers who want to see the merged result: https://github.com/injinj/openclaw/tree/combined/reset-archive-coverage |
Address Greptile review feedback on PR openclaw#71537: * nullglob is now saved and restored inside list_session_transcripts so the helper does not change the caller shell environment when sourced. * Replace 'for f in $(list_session_transcripts)' tip with a 'while IFS= read -r' loop so paths with spaces or other IFS characters are handled correctly. Include a worked example using the same date+size listing the surrounding section already documents. Doc only, no runtime behavior change.
23893a3 to
96f908c
Compare
|
Codex review: needs maintainer review before merge. Reviewed May 29, 2026, 6:40 AM ET / 10:40 UTC. Summary PR surface: Source +30, Tests +34, Docs +60. Total +124 across 3 files. Reproducibility: yes. A high-confidence source reproduction is Review metrics: none identified. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the focused reset-archive recovery after maintainers accept the archive-selection and deleted-archive search semantics and the remaining required checks complete. Do we have a high-confidence way to reproduce the issue? Yes. A high-confidence source reproduction is Is this the best way to solve the issue? Yes, with one maintainer caveat. The runtime fix is narrow and keeps live AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against 68d0c0f2f5c4. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +30, Tests +34, Docs +60. Total +124 across 3 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
|
ClawSweeper PR egg 🎁 Pass real behavior proof to wake the egg and unlock a hatchable treat. Where did the egg go?
|
|
Heads up: this PR needs to be updated against current |
Plain .jsonl globs miss .jsonl.reset.*Z and .jsonl.deleted.*Z files, which still contain real transcript content. Add explicit guidance and a list_session_transcripts helper so searches can opt in to full history when needed. Pairs with the session-memory recover-from-archive fix in the same branch.
Address Greptile review feedback on PR openclaw#71537: * nullglob is now saved and restored inside list_session_transcripts so the helper does not change the caller shell environment when sourced. * Replace 'for f in $(list_session_transcripts)' tip with a 'while IFS= read -r' loop so paths with spaces or other IFS characters are handled correctly. Include a worked example using the same date+size listing the surrounding section already documents. Doc only, no runtime behavior change.
0a487e1 to
1a23104
Compare
Dependency graph guard clearedThis PR no longer has blocked dependency graph changes. A future dependency graph change requires a fresh
|
1a23104 to
9cb848e
Compare
9cb848e to
2edb418
Compare
Summary
When a session is reset (
/newor/reset), the gateway renames its<id>.jsonltranscript to<id>.jsonl.reset.<timestamp>Z. After thatrename, two surfaces silently lose the conversation:
previous conversation into the daily memory file. It only ever looked
at live
.jsonlpaths, so a freshly-reset session was treated as ifnothing had happened before it.
session-logsskill, which agents (and users) consult to grephistorical sessions. Every example used a
*.jsonlglob and quietlyskipped both
.reset.*Zand.deleted.*Zarchives.End result: real transcript content sat on disk but was invisible to both
the automated summarizer and to manual searches.
This branch fixes both halves so archives stop being a black hole.
Changes
1.
fix(session-memory): recover archived reset transcriptssrc/hooks/bundled/session-memory/transcript.tsgetRecentSessionContentWithResetFallbacknow scans the sessionsdirectory for
<base>.reset.<ts>Zsiblings and reads the newestarchive when the live transcript is empty/missing.
findPreviousSessionFilegains four ordered fallbacks for archivedforms:
workflows produce identical results.
`src/hooks/bundled/session-memory/handler.test.ts` adds three test
cases covering: a `.reset.*Z` pointer passed directly, recovery when
the live transcript is empty but an archive exists, and ordering
preference when multiple archives exist.
2. `docs(session-logs): include archived transcripts in skill examples`
`skills/session-logs/SKILL.md`
`.jsonl.reset.Z`, `.jsonl.deleted.*Z`) and warns that plain
globs miss the archived ones.
that emit paths from all three forms.
active-only and active+archived `rg -l` invocations.
explicitly.
Why both in one PR
These changes are two halves of the same bug. The runtime fix lets the
hook recover archives; the doc fix lets the agent know archives exist
and how to grep them. Landing one without the other leaves the other
half silently broken.
Out of scope (intentionally)
scan are unchanged. Their current behavior (treating `.reset.*Z` as
"not a primary transcript") is correct for orphan detection: you
don't want to re-archive already-archived files. Archives become
searchable, not active.
Testing
```
node scripts/run-vitest.mjs run --config test/vitest/vitest.unit.config.ts \
src/hooks/bundled/session-memory/handler.test.ts
```
Real behavior proof
Behavior or issue addressed:
findPreviousSessionFileinsrc/hooks/bundled/session-memory/transcript.tsnever inspects*.jsonl.reset.*archives. When a session reset renames<id>.jsonlto<id>.jsonl.reset.<ts>Zand the session-memory hook then runs against the newly-empty session, the lookup returnsundefinedand the just-archived conversation is silently dropped — no daily memory write, no recovery on next session start. This PR adds four ordered.reset.*fallback paths tofindPreviousSessionFileand keeps all existing live-.jsonlbranches first so unchanged workflows are unaffected.Real environment tested: chex (Fedora 41, kernel 6.19.9, Node 22.22.0, pnpm 11.2.2). Direct execution of the modified
transcript.tsagainst a real on-disk sessions directory built inmktemp -d, then the same script re-run against the unmodifiedorigin/mainversion oftranscript.tsswapped in to capture the regression on current upstream code.Exact steps or command run after this patch:
Repro script (
scripts/repro-reset-recovery.mjs):Evidence after fix: Terminal output captured on chex.
BEFORE (running with
origin/mainversion oftranscript.ts):AFTER (this branch,
9cb848ea08):Vitest run on this branch (terminal output, chex):
All 24 session-memory hook tests pass, including the 4 new tests added in this PR exercising the
.reset.fallback paths.Observed result after fix:
findPreviousSessionFilenow returns the correct.jsonl.reset.<ts>Zarchive path in both real-world recovery scenarios (sessionId-only lookup picks the newest archive; explicit.reset.*currentSessionFileis matched and returned). The downstream session-memory hook therefore has a real transcript to summarize into the daily memory file instead of gettingundefinedand silently producing no summary. Pre-existing live-.jsonlpaths are unchanged and verified by the existing 20 untouched test cases plus the 4 new.resetcases.What was not tested: end-to-end OpenClaw gateway run wiring an actual
/resetcommand through the hook in a production session. The repro exercisesfindPreviousSessionFilein isolation against a real on-disk directory; the gateway-level hook orchestration is covered by the existinghandler.test.tssuite which passes on this branch..deleted.*Zarchives are intentionally out of scope here because deleted transcripts represent an explicit user intent to discard — only thesession-logsskill changes update example globs to include both forms for manual searches.