Skip to content

Commit 86c0622

Browse files
committed
fix(summarize): address CodeRabbit review on #472
Four nits flagged by the automated reviewer, all worth fixing: - scripts/backfill: add curl --connect-timeout + --max-time profiles (META_CURL_OPTS vs WORK_CURL_OPTS). Metadata reads fail fast and retry on transient blips; LLM-backed work calls get a wide 30-min cap and no retry (retrying a half-finished LLM job double-spends). - scripts/backfill: sanitize sessionId before joining with DEBUG_DIR in dump_failure() (otherwise a session id containing `/` or `..` could escape the debug dir). UUIDs in practice, but the server doesn't enforce that. - scripts/backfill: switch the observations query to `--get --data-urlencode "sessionId=$id"` so special characters can't corrupt the query string. - scripts/backfill: guard `jq` on summarize + consolidate responses with `jq -e . </dev/null 2>&1` first. iii's HTTP layer occasionally returns non-JSON (HTML 5xx, empty body on timeout). Without the guard, `set -e` aborts the whole backfill loop on a single bad response — now it logs `invalid_json_response` and moves on. - test/summarize.test.ts: fix `vi.mock("./audit.js", ...)` path to `"../src/functions/audit.js"`. The old path resolved to `test/audit.js` (nonexistent), so the mock was a silent no-op. Tests passed anyway because `safeAudit` writes to a mocked KV. 9/9 tests still pass; backfill dry-run still resolves the corpus cleanly.
1 parent 99cec95 commit 86c0622

2 files changed

Lines changed: 46 additions & 12 deletions

File tree

scripts/backfill-imported-sessions.sh

Lines changed: 45 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -64,17 +64,26 @@ for bin in curl jq; do
6464
command -v "$bin" >/dev/null || { echo "missing dependency: $bin" >&2; exit 1; }
6565
done
6666

67+
# Curl timeout profiles. Metadata reads (livez, sessions list, observations
68+
# pull for debug dumps) should fail fast and retry transient blips. The LLM
69+
# work calls (summarize, consolidate) intentionally have no --retry and a
70+
# wide --max-time: each call can legitimately take minutes for chunked
71+
# summarize on large sessions, and retrying a half-finished LLM job is
72+
# expensive both in dollars and in duplicated server-side work.
73+
META_CURL_OPTS=(--connect-timeout 10 --max-time 30 --retry 2 --retry-delay 1)
74+
WORK_CURL_OPTS=(--connect-timeout 10 --max-time 1800)
75+
6776
echo "agentmemory backfill — server: $URL"
6877
[[ "$DRY_RUN" == 1 ]] && echo "DRY RUN: no POSTs will be made."
6978

7079
# --- liveness ---
71-
if ! curl -fsS "$URL/agentmemory/livez" >/dev/null; then
80+
if ! curl -fsS "${META_CURL_OPTS[@]}" "$URL/agentmemory/livez" >/dev/null; then
7281
echo "server not reachable at $URL (try: npx @agentmemory/agentmemory)" >&2
7382
exit 1
7483
fi
7584

7685
# --- collect session ids ---
77-
sessions_json="$(curl -fsS "$URL/agentmemory/sessions")"
86+
sessions_json="$(curl -fsS "${META_CURL_OPTS[@]}" "$URL/agentmemory/sessions")"
7887
filter='.sessions[] | select(.status=="completed")'
7988
if [[ -n "$ONLY_TAG" ]]; then
8089
filter+=" | select((.tags // []) | index(\"$ONLY_TAG\"))"
@@ -148,13 +157,23 @@ fi
148157

149158
dump_failure() {
150159
local id="$1" obs="$2" resp="$3"
151-
local file="$DEBUG_DIR/${id}.json"
160+
# Replace anything outside [A-Za-z0-9._-] with `_` before joining with
161+
# DEBUG_DIR. Session IDs from the API are UUIDs in practice, but the
162+
# server doesn't enforce that — a hostile or buggy id containing `/` or
163+
# `..` would otherwise escape the debug directory.
164+
local safe_id
165+
safe_id="$(printf '%s' "$id" | tr -c 'A-Za-z0-9._-' '_')"
166+
local file="$DEBUG_DIR/${safe_id}.json"
152167
# Pull the raw observations (what would have gone into the prompt) so the
153168
# operator can reconstruct the upstream payload locally. We also compute
154169
# narrative size stats so size-related rejections are immediately visible.
155170
# Stream observations through stdin (avoids exec-arg overflow on
156171
# multi-thousand-obs sessions — macOS argv ceiling is ~256k).
157-
curl -fsS "$URL/agentmemory/observations?sessionId=$id" \
172+
# `--get --data-urlencode` percent-encodes the session id so special
173+
# characters can't corrupt the query string.
174+
curl -fsS "${META_CURL_OPTS[@]}" --get \
175+
--data-urlencode "sessionId=$id" \
176+
"$URL/agentmemory/observations" \
158177
| jq \
159178
--arg id "$id" \
160179
--argjson obsCount "$obs" \
@@ -185,11 +204,20 @@ for row in "${rows[@]}"; do
185204
obs="$(cut -f2 <<<"$row")"
186205

187206
body="$(jq -nc --arg id "$id" '{sessionId:$id}')"
188-
resp="$(curl -sS -X POST "$URL/agentmemory/summarize" \
207+
resp="$(curl -sS "${WORK_CURL_OPTS[@]}" -X POST "$URL/agentmemory/summarize" \
189208
-H 'content-type: application/json' --data "$body" || echo '{"success":false,"error":"curl_failed"}')"
190-
status="$(jq -r '.success // false' <<<"$resp")"
191-
err="$(jq -r '.error // ""' <<<"$resp")"
192-
title="$(jq -r '.summary.title // ""' <<<"$resp")"
209+
# iii's HTTP layer occasionally returns non-JSON (HTML 5xx, empty body
210+
# on timeout, etc.). Validate before parsing so `set -e` doesn't abort
211+
# the whole backfill loop on a single bad response.
212+
if jq -e . >/dev/null 2>&1 <<<"$resp"; then
213+
status="$(jq -r '.success // false' <<<"$resp")"
214+
err="$(jq -r '.error // ""' <<<"$resp")"
215+
title="$(jq -r '.summary.title // ""' <<<"$resp")"
216+
else
217+
status="false"
218+
err="invalid_json_response"
219+
title=""
220+
fi
193221

194222
if [[ "$status" == "true" ]]; then
195223
ok=$(( ok + 1 ))
@@ -220,6 +248,12 @@ fi
220248

221249
echo
222250
echo "running consolidate-pipeline …"
223-
resp="$(curl -sS -X POST "$URL/agentmemory/consolidate-pipeline" \
224-
-H 'content-type: application/json' --data '{}')"
225-
echo "$resp" | jq .
251+
resp="$(curl -sS "${WORK_CURL_OPTS[@]}" -X POST "$URL/agentmemory/consolidate-pipeline" \
252+
-H 'content-type: application/json' --data '{}' || echo '{"success":false,"error":"curl_failed"}')"
253+
if jq -e . >/dev/null 2>&1 <<<"$resp"; then
254+
echo "$resp" | jq .
255+
else
256+
echo "consolidate-pipeline returned non-JSON (likely a timeout or upstream error):"
257+
printf '%s\n' "$resp" | head -c 500
258+
echo
259+
fi

test/summarize.test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ vi.mock("../src/eval/quality.js", () => ({
2525
scoreSummary: () => 100,
2626
}));
2727

28-
vi.mock("./audit.js", () => ({
28+
vi.mock("../src/functions/audit.js", () => ({
2929
safeAudit: vi.fn(),
3030
}));
3131

0 commit comments

Comments
 (0)