Skip to content

fix(checkpoint): guard _touch_project against non-dict project metadata - #22639

Closed
wesleysimplicio wants to merge 1 commit into
NousResearch:mainfrom
wesleysimplicio:fix/agente-a-checkpoint-touch-project-shape
Closed

fix(checkpoint): guard _touch_project against non-dict project metadata#22639
wesleysimplicio wants to merge 1 commit into
NousResearch:mainfrom
wesleysimplicio:fix/agente-a-checkpoint-touch-project-shape

Conversation

@wesleysimplicio

Copy link
Copy Markdown
Contributor

Problem

tools.checkpoint_manager._touch_project parses the project metadata file with json.loads, then immediately subscript-assigns into the result:

try:
    meta = json.loads(meta_path.read_text(encoding="utf-8"))
except (OSError, ValueError):
    meta = {}
meta["workdir"] = str(_normalize_path(working_dir))  # ← crashes if meta is a list

When the file parses successfully but returns a non-dict ([], null, 42, etc. — from a corrupted or truncated write), json.loads does not raise, the except (OSError, ValueError) clause is skipped, and meta["workdir"] = ... raises TypeError: list indices must be integers or slices, not str.

This TypeError is not caught in _touch_project or _take. It propagates to ensure_checkpoint's broad except Exception safety net, which silently returns False — disabling all checkpoints for the affected working directory for the entire session, with no user-visible error.

Root cause

Missing isinstance(meta, dict) guard after json.loads. The same guard is already present in _list_projects (line 506 of the same file) but was omitted in _touch_project. Same pattern as fixes in cron/jobs.py (#22569) and tools/process_registry.py (#22544).

Fix

     try:
         meta = json.loads(meta_path.read_text(encoding="utf-8"))
     except (OSError, ValueError):
         meta = {}
+    if not isinstance(meta, dict):
+        meta = {}
     meta["workdir"] = str(_normalize_path(working_dir))

Two lines, matching the existing guard in _list_projects.

Tests

TestTouchProjectMalformedMeta covers four non-dict payloads ([], null, 42, "oops"):

  • Writes corrupted metadata to <store>/projects/<hash>.json
  • Calls _touch_project — asserts no exception raised
  • Asserts metadata file is rewritten as a valid dict with last_touch and workdir present

All four cases fail on main with TypeError, pass with the fix. Full tests/tools/test_checkpoint_manager.py regression: 77 passed.

Problem
=======
`tools.checkpoint_manager._touch_project` reads the project metadata
file with `json.loads(meta_path.read_text(...))`, then immediately does:

    meta["workdir"] = str(_normalize_path(working_dir))

The `except` block only catches `(OSError, ValueError)`.  When the file
parses successfully but returns a non-dict value (a list `[]`, `null`,
or a scalar from a corrupted or hand-truncated write), `json.loads`
succeeds without error and `meta` is set to, e.g., `[]`.  The subsequent
subscript assignment then raises `TypeError: list indices must be
integers or slices, not str`, which is NOT caught by the narrow except
clause.

This TypeError propagates up through `_take` to `ensure_checkpoint`,
where the broad `except Exception` safety net swallows it.  The effect
is that `ensure_checkpoint` silently returns False for the entire
session — all checkpoints are skipped for the affected working directory
without any user-visible error.

Root cause
==========
Missing `isinstance(meta, dict)` guard after `json.loads`, identical in
pattern to bugs fixed in `cron/jobs.py` (NousResearch#22569) and
`tools/process_registry.py` (NousResearch#22544).  The same guard is already
present one function below in `_list_projects` (line 506), but was
inadvertently omitted in `_touch_project`.

Fix
===
Add two lines after the try/except:

```python
if not isinstance(meta, dict):
    meta = {}
```

This matches the existing guard in `_list_projects` and ensures a fresh
empty dict is used whenever the persisted value is not a mapping —
preserving the `created_at` semantics via `setdefault` on the next line.

Tests
=====
`TestTouchProjectMalformedMeta` covers four non-dict root values
(`[]`, `null`, `42`, `"oops"`).  Each writes a corrupted metadata file,
calls `_touch_project`, and asserts: (a) no exception raised, (b) the
metadata file is rewritten as a valid dict containing `last_touch` and
`workdir`.  All four fail on main with `TypeError`, pass with fix.
Full `tests/tools/test_checkpoint_manager.py` regression: 77 passed.
Copilot AI review requested due to automatic review settings May 9, 2026 15:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR hardens tools.checkpoint_manager._touch_project against corrupted-but-JSON-valid project metadata by ensuring the parsed metadata is a dict before performing key assignments, preventing a TypeError that could silently disable checkpointing for a session.

Changes:

  • Add a post-json.loads shape guard in _touch_project to coerce non-dict payloads to {}.
  • Add a regression test suite covering several non-dict JSON payloads and asserting _touch_project repairs the metadata file.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
tools/checkpoint_manager.py Adds isinstance(meta, dict) guard to prevent TypeError when metadata JSON root is not an object.
tests/tools/test_checkpoint_manager.py Adds parametrized regression tests for malformed metadata shapes to ensure _touch_project does not raise and rewrites valid metadata.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +616 to +622
When ``json.load`` succeeds but returns a non-dict (e.g. a list ``[]``,
``null``, or a scalar), the subsequent ``meta["workdir"] = ...`` raises
``TypeError: list indices must be integers…``. This TypeError propagates
uncaught out of ``_touch_project`` and up through ``_take`` into
``ensure_checkpoint``, where it is swallowed by the broad ``except
Exception`` safety net — but the effect is that the checkpoint is silently
skipped for the entire session.
@teknium1

Copy link
Copy Markdown
Contributor

Merged via salvage PR #22848. salvage cherry-picked your commit; authorship preserved. Thanks for the contribution!

@teknium1 teknium1 closed this May 10, 2026
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.

3 participants