Skip to content

fix(context_engine): pair pinned skill results by position - #913

Open
LivXue wants to merge 1 commit into
mainfrom
fix/curator_skill_pin_pairing
Open

LivXue wants to merge 1 commit into
mainfrom
fix/curator_skill_pin_pairing

Conversation

@LivXue

@LivXue LivXue commented Oct 11, 2026

Copy link
Copy Markdown
Member

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

  • Fix
  • Feature
  • Docs
  • CI / tooling
  • Refactor
  • Other

Verification

  • Relevant tests pass locally
  • Relevant lint / type checks pass locally
  • User-facing docs or screenshots are updated when needed

Native Windows, Python 3.12. PYTHONUTF8=1; the existing environment was selected through UV_PROJECT_ENVIRONMENT, and PYTHONPATH pointed at this checkout.

uv run --no-sync pytest -o addopts= tests/test_curator_context_engine.py tests/test_history_trimmer.py -q --tb=short --show-capture=no

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:

$changed = @(git diff --name-only origin/main...HEAD -- '*.py')
$production = @($changed | Where-Object { $_ -like 'raven/*' })
uv run --no-sync ruff check @changed
uv run --no-sync ruff format --check @changed
uv run --no-sync ty check @production
uv run --no-sync lint-imports
uv run --no-sync pre-commit run --from-ref origin/main --to-ref HEAD
uv run --no-sync python scripts/check_commit_messages.py origin/main..HEAD
uv run --no-sync python scripts/check_source_language.py origin/main...HEAD
uv run --no-sync python scripts/check_large_files.py origin/main...HEAD
git diff --check origin/main...HEAD

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

  • Security impact considered
  • Backward compatibility considered
  • Rollback path is clear for risky changes

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

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>
@LivXue
LivXue requested review from 0xKT and gloryfromca October 11, 2026 16:27

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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 after uv sync --all-packages. The first run had 42 setup errors because the workspace plugin raven_everos was 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 (the make wrapper was unavailable in this environment).

@LivXue

LivXue commented Oct 11, 2026

Copy link
Copy Markdown
Member Author

Windows compatibility review: no new issue found in the inspected scope.

Reviewed head ee2d8aea1ecc41526e3ee2453859df1422c5a45a against base 4acc63cebd0752652194f9e4989628da29a8a25d: both changed files, the shared positional pairing helper, adjacency closure, and manifest caller. Checked reused and distinct call IDs, latest-fetch selection, sibling results, and the existing manifest/archive behavior.

Native Windows / Python 3.13.12, in a detached temporary checkout with PYTHONUTF8=1, UV_NO_SYNC=1 and isolated user/Raven homes:

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.

@LivXue

LivXue commented Oct 11, 2026

Copy link
Copy Markdown
Member Author

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 ee2d8aea1ecc. This review covers all changed areas, not only raven/agent.

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):

  • PR head: uv run --no-sync pytest -o addopts= tests/test_curator_context_engine.py tests/test_history_trimmer.py -q --tb=short -> 42 passed.

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.

git diff --check over the complete merge-base-to-head range passed. No new inline finding is being filed.

This branch has not been deployed

No deployments
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.

2 participants