Repository navigation
Conversation
A tool-call ID reused later in a session made skill pinning retain unrelated results or a superseded fetch. Use the same positional parent relation as HistoryTrimmer so the pin and adjacency closure agree. Co-authored-by: Codex <noreply@openai.com>
gloryfromca
left a comment
There was a problem hiding this comment.
No blockers; this can merge as far as I am concerned.
Reviewed the diff, the pinned-message caller and manifest/archive flow, the shared positional pairing implementation and its history, and the Context Engine terminology/architecture rules. I also checked backward compatibility and malformed-input coverage, and confirmed the tests add the reused-ID regression without weakening existing assertions.
Verification:
uv run pytest tests/test_curator_context_engine.py tests/test_history_trimmer.py -q: 42 passed afteruv sync --all-packages. The first run had 42 setup errors because the workspace pluginraven_everoswas not installed; no test body ran on that attempt.uv run ruff check raven/context_engine/curator.py tests/test_curator_context_engine.py: passed.git diff --check github/main...HEAD: passed.uv run python scripts/check_source_language.py github/main...HEAD: passed (themakewrapper was unavailable in this environment).
|
Windows compatibility review: no new issue found in the inspected scope. Reviewed head Native Windows / Python 3.13.12, in a detached temporary checkout with uv run pytest tests/test_curator_context_engine.py tests/test_history_trimmer.py -n 0 -x --tb=short --basetemp "$reviewRunDir/pr913/tests"Result: 42 passed. No live provider or full repository suite was run. One non-failing idle-ceiling warning occurred during shared setup; strict idle checking was not enabled. This scoped review does not establish full Windows compatibility. |
|
No blockers in the reviewed scope. Scope: I reviewed the full PR diff (2 changed files), including tests and documentation, and traced affected callers and contracts at Checked positional tool-result pairing against canonical_ids and tool_group, repeated call IDs, the latest fetch of each pinned skill, sibling-result closure, malformed calls, empty pin sets, and manifest readback. The pinning path now uses the same parent relation as the trimmer. Verification (native Windows, Python 3.12, isolated source snapshots; local JUnit-output options omitted below):
The tests cover the reused-ID behavior through both pinning and manifest construction, and check compatibility with trimmer closure. No new finding was identified. I also read the Python 3.13 Windows review. Its conclusions are consistent; the measurements here record a separate Python 3.12 run.
|
Summary
A tool-call ID reused later in a session made skill pinning retain unrelated results or a superseded fetch. Use the same positional parent relation as HistoryTrimmer so the pin and adjacency closure agree.
Keep sibling results of the pinned assistant turn and only the latest fetch per skill. Reused-ID and distinct-ID controls check the pin set, closed selection and manifest.
Type
Verification
Native Windows, Python 3.12. PYTHONUTF8=1; the existing environment was selected through UV_PROJECT_ENVIRONMENT, and PYTHONPATH pointed at this checkout.
Result: 42 passed in the targeted selection. Both complete selected files passed. Restoring the prior pin logic made the two reused-ID cases fail while distinct-ID controls passed. No token-estimation or trimming-budget policy change is included.
Whole-branch gates:
All listed gates passed. The whole-branch review covered callers, ownership, guard boundaries, failure paths, regression strength, architecture and compatibility. No screenshot change applies; directly affected code documentation is updated where needed. The full repository suite and Linux/macOS execution were not run locally; the local Docker Linux engine was unavailable.
Risk
Histories with reused tool-call IDs can retain fewer unrelated messages, matching positional adjacency. The complete pinned exchange and latest-fetch rule remain intact. Revert the squash commit to roll back; there is no persisted format change.
Related Issues
#885