fix(storage): skip non-string keys in SnapshotVars 🤖🤖🤖

[sushant-mishra-dtu]

What this fixes

SnapshotVars exists for one reason, stated in its own module docstring
(src/nooa/storage/snapshot_vars.py:6-10):

A single non-serializable value used to abort the whole snapshot, silently losing all
durable state on the next resume. SnapshotVars moves that check to write time.

It checks the value on write and skips it. It never checks the key
(src/nooa/storage/snapshot_vars.py:57-69):

def __setitem__(self, key: str, value: Any) -> None:
    try:
        serialize(value)                     # value is validated
    except (SerializationError, TypeError, ValueError, RecursionError) as exc:
        logger.warning(...)
        return
    self._data[key] = value                  # key is not

Snapshots are JSON, and serialize() rejects non-string dict keys outright
(src/nooa/storage/serialization.py:124-128). So a single non-string key does the exact
thing this class was built to prevent: it survives the write, then blows up at snapshot
time, where AgentSnapshot.from_agent() catches SerializationError and drops the whole
attribute (src/nooa/storage/snapshot.py:109-121) -- taking every other var with it.
The comment right there names the consequence exactly (snapshot.py:112-114):

A single non-serializable attribute must not abort the whole snapshot -- that silently
loses ALL durable state (vars, todos, ...).

That guard is doing its job. SnapshotVars is the layer meant to stop anything reaching it,
and for keys it does not.

Why it matters

self.vars is agent-writable surface. It is InteractiveAgent's persistent variable store
(src/nooa/interactive.py:346, :351), documented as "survives across turns AND across
sessions", and it is the type of Todo.vars (src/nooa/tools/todo.py:77), which coerces
any dict handed to it (:90). An agent writing self.vars[step] = state with an integer
step, or Todo(vars={1: "..."}), gets a container that looks like it stored the value.

The loss is silent from the agent's side: the write succeeds, the read succeeds, and the
warning that fires is at snapshot time, about vars as a whole, not about the key that
caused it. On the next /exit + resume every durable variable is gone.

Reproduction

from nooa.interactive import InteractiveAgent
from nooa.storage.snapshot import AgentSnapshot

a = InteractiveAgent()
a.vars["safe_var"] = "valuable state"
a.vars[123] = "bad key"
snap = AgentSnapshot.from_agent(a)
print("'vars' in snapshot attributes:", "vars" in snap.attributes)

On main:

WARNING nooa.storage.snapshot: Snapshot: skipping non-serializable attribute 'vars'
        (SnapshotVars): Dict key 123 (type: int) is not a string. JSON requires string keys.
vars container holds: {'safe_var': 'valuable state', 123: 'bad key'}
'vars' in snapshot attributes: False
snapshot attribute names: []

safe_var was never at fault and is lost anyway. On this branch:

WARNING nooa.storage.snapshot_vars: SnapshotVars: key 123 (int) is not a string and will
        NOT be persisted (it won't survive /exit + resume): snapshots are JSON, which
        requires string keys
vars container holds: {'safe_var': 'valuable state'}
'vars' in snapshot attributes: True
snapshot attribute names: ['vars']

The fix

Validate the key the way the value is already validated -- warn, name the offending key,
skip the store:

def __setitem__(self, key: str, value: Any) -> None:
    if not isinstance(key, str):
        logger.warning(
            "SnapshotVars: key %r (%s) is not a string and will NOT be persisted "
            "(it won't survive /exit + resume): snapshots are JSON, which requires "
            "string keys",
            key,
            type(key).__name__,
        )
        return
    ...

Nine lines, no signature change. It matches the existing key: str annotation and the
class's documented "skips the store, logs a warning" contract, so the behaviour is the one
already described in the docstring. It also moves the warning to the write that caused it,
which is where a user can act on it.

Test

One test, test_snapshot_serialize_succeeds_even_after_non_string_key_write, placed beside
the existing test_snapshot_serialize_succeeds_even_after_bad_write it mirrors. Verified to
fail on the unfixed tree before being kept:

FAILED tests/storage/test_snapshot_vars.py::TestSnapshotVarsRoundTrips::
       test_snapshot_serialize_succeeds_even_after_non_string_key_write
E   nooa.errors.storage.SerializationError: Dict key 123 (type: int) is not a string.
    JSON requires string keys.

tests/storage/test_snapshot_vars.py is 15 passed on this branch against 14 on main.

Scope

Only the missing key check. Nothing else in the container changes: reads, iteration,
deletion and the value path are untouched, and a non-string key was never retrievable from
a restored snapshot anyway -- it was only ever taking the rest of the snapshot down with it.

Summary by CodeRabbit

• Bug Fixes
  • Snapshot storage now skips entries with non-string keys, preventing serialization issues while preserving valid entries.
  • A warning is logged when an unsupported key is provided.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: c6379cdb-aa43-4c10-b7cd-d44e78dd8ba2

📥 Commits

Reviewing files that changed from the base of the PR and between e137e1b and 243a4ba.

📒 Files selected for processing (2)

• src/nooa/storage/snapshot_vars.py
• tests/storage/test_snapshot_vars.py

Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review.

---

📝 Walkthrough

Walkthrough

SnapshotVars now skips non-string keys before serialization and logs a warning. A regression test confirms that valid entries remain serialized after an invalid key write.

Changes

Snapshot key validation

Layer / File(s)	Summary
Validate and test snapshot keys src/nooa/storage/snapshot_vars.py, tests/storage/test_snapshot_vars.py	__setitem__ rejects and logs non-string keys. The regression test confirms that valid entries remain in serialized output.

Estimated code review effort: 1 (Trivial) | ~5 minutes

Merge Risk: ⚪ Minimal · up to 243a4

Snapshots now ignore invalid non-string variable keys rather than allowing them to disrupt serialization of valid variables. The targeted behavior is covered, with no remaining merge-blocking risk identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the main change: SnapshotVars skips non-string keys. The extra emojis add minor noise but do not obscure the change.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}