github-stale-ci-pr-closer: KV state is double-wrapped, never persists, and hits the depth-32 limit

The bug is in skills/github-stale-ci-pr-closer/scripts/worker.py: _kv_request wraps the body as {"value": ...} around line 82, and load_state reads that wrapper back. Run the bundled repro_stale_ci_kv.py with --worker-file pointing at your edited worker.py and check that stored depth stays near 4 and the PR record carries over between runs. Done when the script exits 0 and prints the round-trip OK result.

Summary

github-stale-ci-pr-closer wraps its state in {"value": state} before PUT /v1/kv/{key}, but the KV API stores the entire request body as the value. Every save therefore nests the state one level deeper. Two consequences:

1. State never persists between runs. load_state returns {"value": <previous state>}, which has no top-level prs, so every run starts with empty per-PR records (warning timestamps, author follow-up resets are lost).
2. The automation eventually fails on every run. After ~28 saves the value exceeds the KV store's nesting limit and every save_state gets HTTP 400 invalid_value: Value exceeds maximum nesting depth of 32. The worker logs only {"repository": ..., "error": "HTTPError"}, so the cause is invisible.

Observed in production on a self-hosted deployment: 215 consecutive hourly runs failed for all configured repositories with exactly that 400.

Where

skills/github-stale-ci-pr-closer/scripts/worker.py on main (7cec6d3):

def _kv_request(key, method, value=None):
    ...
    data = json.dumps({"value": value}).encode() if value is not None else None   # line 82
    ...
            return payload.get("value")

The server side (OpenHands/automation openhands/automation/kv_router.py, both PyPI 1.19.0 and main bc4cca7):

@router.put("/{key}")
async def set_value(
    key: ValidatedKey,
    body: Annotated[Any, Body()],  # Accept any JSON body directly as the value
    ...
    """Set a value for a key.

    The entire request body is stored as the value.

This matches the documented contract (docs/kv-store-client-guide.md: curl -X PUT ... -d '{"foo": "bar"}') and every other KV client in this repo (e.g. the PR reviewer's agent_conversation._kv_request sends json.dumps(value)). github-stale-ci-pr-closer is the only skill that wraps the body.

Reproduction

Self-contained: runs the real automation-service KV router on a throwaway SQLite DB and drives the real load_state / save_state from worker.py through it, one call pair per simulated hourly run. No GitHub token, no Postgres, no checkout.

# save the script below as repro_stale_ci_kv.py, then:
uv run --no-project --with openhands-automation==1.19.0 --with uvicorn \
    python repro_stale_ci_kv.py --worker-ref main

Expected output (abridged), exit code 1:

 run  record from previous run?  stored depth  stored top-level keys
   1  False                                 5  ['value']
   2  False                                 6  ['value']
 ...
  28  False                                32  ['value']
  29  False                                 -  save_state -> HTTP 400: {"detail":"invalid_value: Value exceeds maximum nesting depth of 32"}

- PR record written by the previous run was missing on 28 run(s)
- save_state failed on run 29 with HTTP 400: {"detail":"invalid_value: Value exceeds maximum nesting depth of 32"}
RESULT: BUG REPRODUCED

The same script verifies a fix (exit code 0):

# a fixed worker on a fresh key: depth stays 4, the record carries over every run
... python repro_stale_ci_kv.py --worker-file path/to/fixed/worker.py

# existing installations: run the current worker until it breaks, then upgrade on the same key
... python repro_stale_ci_kv.py --worker-ref main --upgrade-to path/to/fixed/worker.py

Also reproduces against the automation service built from source:
--with "openhands-automation @ git+https://github.com/OpenHands/automation@bc4cca7".

"""Reproduce: github-stale-ci-pr-closer double-wraps its KV state.

Runs the REAL automation-service KV router (from the openhands-automation
package) on a throwaway SQLite DB, then drives the REAL `load_state` /
`save_state` from skills/github-stale-ci-pr-closer/scripts/worker.py through
it, once per simulated hourly run. No GitHub access, no Postgres, no checkout.

    uv run --no-project --with openhands-automation==1.19.0 --with uvicorn \
        python repro_stale_ci_kv.py                       # worker.py from extensions@main

    ... python repro_stale_ci_kv.py --worker-ref <sha>    # any extensions ref
    ... python repro_stale_ci_kv.py --worker-file w.py    # a local (fixed) worker
    ... python repro_stale_ci_kv.py --upgrade-to w.py     # buggy until it fails, then
                                                          # switch to w.py on the same key

Exit code reflects the last phase run: 1 = bug reproduced, 0 = state
round-trips and stays flat.
"""

import argparse
import asyncio
import json
import os
import secrets
import socket
import sys
import tempfile
import threading
import time
import types
import urllib.parse
import urllib.request
import uuid
from urllib.error import HTTPError

ap = argparse.ArgumentParser()
ap.add_argument("--worker-ref", default="main", help="OpenHands/extensions git ref for worker.py")
ap.add_argument("--worker-file", help="use a local worker.py instead of downloading one")
ap.add_argument("--upgrade-to", help="after the first phase, continue with this worker.py")
ap.add_argument("--runs", type=int, default=40)
args = ap.parse_args()

# --- 1. Configure and start the real KV router -----------------------------
os.environ["AUTOMATION_KV_SECRET"] = secrets.token_urlsafe(32)
tmpdir = tempfile.mkdtemp(prefix="kv-repro-")
os.environ["AUTOMATION_DB_URL"] = f"sqlite+aiosqlite:///{tmpdir}/kv.db"
os.environ["OPENHANDS_SUPPRESS_BANNER"] = "1"

import logging  # noqa: E402

import uvicorn  # noqa: E402
from fastapi import FastAPI  # noqa: E402
from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine  # noqa: E402

import openhands.automation.db as oh_db  # noqa: E402
from openhands.automation.auth import create_http_client  # noqa: E402
from openhands.automation.kv_router import router as kv_router  # noqa: E402
from openhands.automation.models import Base  # noqa: E402
from openhands.automation.utils.kv import create_kv_token  # noqa: E402

logging.disable(logging.WARNING)  # the package logs ORM setup at INFO
oh_db._is_sqlite = True  # same branch the service takes in SQLite (local) mode
engine = create_async_engine(os.environ["AUTOMATION_DB_URL"])


async def _create_schema():
    async with engine.begin() as conn:
        await conn.run_sync(Base.metadata.create_all)


asyncio.run(_create_schema())

app = FastAPI()
app.state.session_factory = async_sessionmaker(engine, expire_on_commit=False)
app.state.http_client = create_http_client()  # the real app does this at startup
app.include_router(kv_router, prefix="/api/automation")

with socket.socket() as s:
    s.bind(("127.0.0.1", 0))
    port = s.getsockname()[1]
server = uvicorn.Server(uvicorn.Config(app, host="127.0.0.1", port=port, log_level="warning"))
threading.Thread(target=server.run, daemon=True).start()
while not server.started:
    time.sleep(0.05)

automation_id = uuid.uuid4()
os.environ["AUTOMATION_API_URL"] = f"http://127.0.0.1:{port}/api/automation"
os.environ["AUTOMATION_KV_TOKEN"] = create_kv_token(
    os.environ["AUTOMATION_KV_SECRET"], automation_id, uuid.uuid4()
)
os.environ["AUTOMATION_EVENT_PAYLOAD"] = json.dumps({"automation_id": str(automation_id)})

# load_state/save_state do not touch GitHub; stub the sibling import only.
stub = types.ModuleType("github_client")
stub.GitHubRepository = object
stub.run_repositories = lambda *a, **k: None
sys.modules["github_client"] = stub


# --- 2. Load a worker.py ----------------------------------------------------
def load_worker(path=None, ref="main"):
    if path:
        src, origin = open(path).read(), path
    else:
        origin = (
            "https://raw.githubusercontent.com/OpenHands/extensions/"
            f"{ref}/skills/github-stale-ci-pr-closer/scripts/worker.py"
        )
        src = urllib.request.urlopen(origin, timeout=30).read().decode()
    module = types.ModuleType("worker")
    exec(compile(src, origin, "exec"), module.__dict__)
    return module, origin


# --- 3. Helpers: read the stored value exactly as the service holds it -----
def stored_value(key):
    req = urllib.request.Request(
        f"{os.environ['AUTOMATION_API_URL']}/v1/kv/{urllib.parse.quote(key, safe='')}",
        headers={"Authorization": f"Bearer {os.environ['AUTOMATION_KV_TOKEN']}"},
    )
    with urllib.request.urlopen(req) as r:
        return json.loads(r.read())["value"]


def depth(v, d=0):
    """Same measure as the service's _check_nesting_depth."""
    if isinstance(v, dict):
        return max([depth(x, d + 1) for x in v.values()] or [d + 1])
    if isinstance(v, list):
        return max([depth(x, d + 1) for x in v] or [d + 1])
    return d


REPO = "OpenHands/example"
PR = "1234"


def simulate(worker, origin, runs, first_run=1, expect_existing=False):
    """One loop iteration = what one hourly run does with its state."""
    key = worker._state_key(REPO)
    print(f"\nworker.py: {origin}\nkey:       {key}\n")
    print(f"{'run':>4}  {'record from previous run?':<26} {'stored depth':>12}  stored top-level keys")
    lost, failed = 0, None
    for run in range(first_run, first_run + runs):
        try:
            state = worker.load_state(REPO)
        except HTTPError as exc:
            sys.exit(f"setup error: load_state -> HTTP {exc.code}: {exc.read().decode()}")
        records = state.setdefault("prs", {})
        carried = PR in records
        if (run > 1 or expect_existing) and not carried:
            lost += 1
        records[PR] = {
            "head_sha": "deadbeef",
            "first_failed_at": 1_700_000_000,
            "warning": {"at": 1_700_600_000, "head_sha": "deadbeef", "comment_id": 42},
        }
        try:
            worker.save_state(REPO, state)
        except HTTPError as exc:
            failed = (run, exc.code, exc.read().decode())
            print(f"{run:>4}  {str(carried):<26} {'-':>12}  save_state -> HTTP {exc.code}: {failed[2]}")
            break
        v = stored_value(key)
        print(f"{run:>4}  {str(carried):<26} {depth(v):>12}  {sorted(v)}")
    print()
    if lost:
        print(f"- PR record written by the previous run was missing on {lost} run(s)")
    if failed:
        print(f"- save_state failed on run {failed[0]} with HTTP {failed[1]}: {failed[2]}")
    bug = bool(failed) or lost > 0
    print("RESULT:", "BUG REPRODUCED" if bug else "OK - state round-trips and stays flat")
    return bug, (failed[0] if failed else first_run + runs)


print(f"service:   openhands-automation KV router (real), SQLite at {tmpdir}")
worker, origin = load_worker(args.worker_file, args.worker_ref)
bug, next_run = simulate(worker, origin, args.runs)
if args.upgrade_to:
    print("\n=== upgrading to", args.upgrade_to, "on the same KV key ===")
    upgraded, origin = load_worker(args.upgrade_to)
    bug, _ = simulate(upgraded, origin, 5, first_run=next_run, expect_existing=True)

server.should_exit = True
sys.exit(1 if bug else 0)

Notes on the harness: it mounts only the KV router in a minimal FastAPI app (the real app.py lifespan needs an agent server), creates the schema with Base.metadata.create_all, and stubs the github_client import, which load_state/save_state do not use. SQLite foreign keys are not enabled, so no automations row is needed.

Proposed fix

Send the value itself, and make load_state unwrap the legacy nesting so installations that already have deep state recover on their next run (verified with --upgrade-to above: the first fixed run reads the latest records back and the stored depth drops to 4):

--- a/skills/github-stale-ci-pr-closer/scripts/worker.py	2026-10-16 10:31:01
+++ b/skills/github-stale-ci-pr-closer/scripts/worker.py	2026-10-16 10:31:01
@@ -79,7 +79,7 @@
 def _kv_request(key, method, value=None):
     if not KV_TOKEN or not KV_BASE:
         return None
-    data = json.dumps({"value": value}).encode() if value is not None else None
+    data = json.dumps(value).encode() if value is not None else None
     request = Request(
         f"{KV_BASE}/v1/kv/{quote(key, safe='')}",
         data=data,
@@ -101,7 +101,17 @@
 
 def load_state(repo):
     if KV_TOKEN and KV_BASE:
-        return _kv_request(_state_key(repo), "GET") or {"version": 1, "prs": {}}
+        state = _kv_request(_state_key(repo), "GET") or {}
+        # Earlier versions PUT {"value": state}, which the KV store saved as-is,
+        # nesting one level per run; the newest records sit just inside it.
+        while isinstance(state, dict) and "prs" not in state and isinstance(
+            state.get("value"), dict
+        ):
+            state = state["value"]
+        state.pop("value", None)
+        state.setdefault("version", 1)
+        state.setdefault("prs", {})
+        return state
     path = _state_path(repo)
     if not path.exists():
         return {"version": 1, "prs": {}}

Also worth doing in the same change: log the HTTP status and URL in run_repositories instead of only type(exc).__name__, so a failing KV or GitHub call is diagnosable from the run output.

Acceptance criteria

• _kv_request PUTs the raw state (json.dumps(value)), matching the KV API contract.
• load_state recovers state written by the current version ({"value": ...} nesting, up to the depth limit) and returns the newest prs records.
• The repro exits 0 for --worker-file <fixed worker> and for --worker-ref <old ref> --upgrade-to <fixed worker>.
• A unit test round-trips state through a fake KV with raw-body semantics over several runs and asserts records persist and depth stays constant.
• Per-repository failures in run_repositories log the HTTP status (and URL) of the failing request.

OpenHands AI triage

The following comments and acceptance criteria were added by the OpenHands AI agent.

Triage

This is a real functional defect in skills/github-stale-ci-pr-closer/scripts/worker.py. _kv_request PUTs {"value": state}, but the automation KV API stores the request body verbatim (set_value: "The entire request body is stored as the value"), so each run nests the state one level deeper. Two effects follow: no per-PR record survives a run, and after roughly 28 runs save_state fails permanently with invalid_value: Value exceeds maximum nesting depth of 32. The double wrap is the only place this skill deviates from the KV contract: the other clients in this repo (skills/github/scripts/agent_conversation.py, used by github-pr-reviewer) send the raw value, matching docs/kv-store-client-guide.md in OpenHands/automation. The claimed production impact (215 consecutive hourly failures on a self-hosted deployment) is consistent with this code path, though it is reported by the author and not independently attached as logs.

Bounded scope: PUT the raw state and make load_state recover state already stored by the current version so existing installations heal on their next run; also surface the HTTP status and URL of per-repository failures so a failing KV or GitHub call is diagnosable. Non-goals: changing the KV API or its contract in OpenHands/automation; changing the warning/close lifecycle, comment markers, or timing windows; changing the file-based fallback path or _state_key format; out-of-band cleanup or rewriting of already-stored deep values.

Acceptance Criteria

• save_state sends the state object itself as the PUT body (json.dumps(value)), with no {"value": ...} wrapper, matching the contract used by the repo's other KV clients.
• Across at least 30 consecutive simulated hourly runs against the real KV router (the issue's repro_stale_ci_kv.py with a fixed worker at a fresh key), the stored value's nesting depth stays constant, save_state never returns HTTP 400, and the PR record written by the previous run is read back on every run; the repro exits 0.
• load_state returns the prs records written by the previous run rather than a wrapper whose only top-level key is value.
• load_state recovers a legacy value already stored by the current version: given a value nested up to the depth limit whose innermost object contains prs, it returns the newest prs records, drops the legacy value key, and the next save_state writes a flat value. Verified by the issue's upgrade path (run the current worker until it fails, then the fixed worker on the same key), which exits 0.
• load_state does not raise when the stored value is not a dict (for example a bare list or scalar); it returns the default {"version": 1, "prs": {}} so an unexpected value cannot crash a run.
• The file-based fallback path (_state_path), the state key format, and the warning/close lifecycle are unchanged, and the existing skills/github-stale-ci-pr-closer/tests/test_worker.py suite passes.
• A new unit test round-trips state through a fake KV with raw-body semantics across several runs, asserting per-PR records persist and the stored nesting depth stays constant.
• Per-repository failures in run_repositories log the HTTP status and request URL of the failing call instead of only the exception class name. This touches shared skills/github/scripts/github_client.py, so the aggregated Automation failed for: ... behavior and other consumers' behavior must remain unchanged.