Skip to content

fix(context): restore skills after compaction - #9043

Open
Premshay wants to merge 1 commit into
kirodotdev:mainfrom
Premshay:upstream/native-compaction-context-recovery
Open

fix(context): restore skills after compaction#9043
Premshay wants to merge 1 commit into
kirodotdev:mainfrom
Premshay:upstream/native-compaction-context-recovery

Conversation

@Premshay

@Premshay Premshay commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

Provider-native and manual ACP compaction replace the model context but do not arm the existing one-shot skills-context reinjection. The next user turn can therefore no longer discover the skills available to the session.

Why it matters

Compaction is meant to preserve an ongoing session. Losing the skills index after it completes makes tool and workflow discovery depend on a restart rather than the normal next turn.

What changed

Arm the existing mark_needs_reinjection flag when a native compaction completes, including immediate Claude completion, deferred Kiro completion, and provider-emitted completion status. The next turn consumes the flag through the established prompt-building path.

Pattern harvest

When a provider mutates or replaces its context outside the normal session lifecycle, rearm durable session-start context at the completion event. Reuse the existing one-shot reinjection seam rather than duplicating prompt assembly in each provider path.

Tests

PYTHONPATH=src /home/prems/dev/repos/premshay/KiroCrew/.venv/bin/pytest -q test/test_dashboard_chat.py -n0 — 767 passed.

What changed (motivation → approach → change)

N/A — covered by the existing ## What changed section.

Manual verification

N/A — focused automated coverage is sufficient.

Related Issues

N/A.

Checklist

  • Existing tests pass and regression coverage is included.
  • Self-review completed; code follows project style guidelines.
  • Documentation updated where applicable.
  • No secrets, credentials, or internal references in the diff.

Contribution License Agreement

N/A — template placeholder; no CLA wording is supplied.

@Premshay
Premshay requested a review from a team as a code owner September 6, 2026 14:38
@Premshay
Premshay requested a review from bolichen97 September 6, 2026 14:38
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention labels Sep 6, 2026
@iamwhatever

Copy link
Copy Markdown
Collaborator

👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated.

Missing sections:

  • ## Problem / Motivation
  • ## Why it matters
  • ## What changed

Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle.

1 similar comment
@dwu96

dwu96 commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated.

Missing sections:

  • ## Problem / Motivation
  • ## Why it matters
  • ## What changed

Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle.

Provider-native compaction drops session-start context. Re-arm the existing one-shot skills reinjection after confirmed success so the next turn retains the available skills.
@Premshay
Premshay force-pushed the upstream/native-compaction-context-recovery branch from 35c7c9b to df70886 Compare September 6, 2026 15:07
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed df70886a9e337ed7304874c75f843d16ded51620 via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] df70886

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed df70886a9e337ed7304874c75f843d16ded51620 via the fork AI-review pipeline; updated in place on each push.

Review details

I've independently traced the change: the _restore_skills_context_after_compaction() helper arms a one-shot needs_context_reinjection flag (session_compaction.py:221), consumed exactly once at the next turn's message build (chat_runner.py:7152). The arming is idempotent, all three call sites fire only on a completed compaction (mid-turn status, claude /compact, and kiro deferred-wait completed), the failure branch correctly does not arm, the helper degrades gracefully if state.sessions is unset, and session_key is stable across arm/consume. No crash, data-loss, removed-guard, or security path exists in the added lines, and nothing here is a blocking: true AUTOSDE violation. The candidate list was empty and I could not ground a new finding to the required bar.

No findings.

[OPUS-REVIEWED] df70886

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of df70886a9e337ed7304874c75f843d16ded51620 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Correct reuse of the existing one-shot reinjection seam at the three real completion channels; tests pin each path plus the failure negative.

Watch

  • The consuming side of the seam (needs_reinjectionContextBuilder.build_message) exists only in the dashboard runner; messaging surfaces (Slack/Discord) render EVENT_COMPACTION_STATUS but never consume the flag, so the same "skills index lost after compaction" harm persists there. Preexisting gap, not widened here — but the fix's title reads broader than its scope, so worth a follow-up issue for the messaging prompt path.

[DESIGN-REVIEWED] df70886

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of df70886a9e337ed7304874c75f843d16ded51620 via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All evidence is gathered. The fix reuses the existing one-shot mark_needs_reinjection seam (defined at session.py:2053, consumed only at chat_runner.py:7152); the existing arming site at session_compaction.py:636 covers only SessionManager-initiated compaction, so the three dashboard paths were genuinely unarmed. The three insertion sites are mutually exclusive branches. The one depth finding: the same root cause (compaction replaces context, always-on skills from build_message are never re-delivered) exists uncovered on every messaging surface — I counted 17 wait_for_compaction/EVENT_COMPACTION_STATUS completion sites across 12 files (slack, discord, telegram, teams, webex, wecom, weixin, whatsapp, imessage, feishu, task_executor, messaging/driver) — but the consume seam exists only in the dashboard runner, so the general fix is genuinely larger and this is accepted-and-deferred, not a blocker.

First-Principles-Verdict: CONCERNS

Right seam, right level for the dashboard — but the identical lost-skills cause survives, uncounted and unmentioned, on every messaging surface.

What this change ships

Intent: after a compaction completes, the next dashboard turn re-delivers the skills index instead of losing it until restart — a FIX.

  1. Provider-native mid-turn compaction now rearms skills reinjection for the next turn — justified.
  2. Manual /compact on the Claude fallback branch rearms it — justified.
  3. Deferred kiro /compact completion rearms it — justified.
  4. A failed arming is swallowed with a log warning instead of failing the turn — undeclared, minor.

Watch

  • Point patch with counted unfixed siblings: the same root cause — compaction replaces context, and build_message (context.py:3053) only ships always-on skills on new sessions — is live at 17 uncovered completion sites in 12 files (grep wait_for_compaction|EVENT_COMPACTION_STATUS: slack/handler.py:2332, discord:1775, telegram:3201, teams:1083+1132, webex:1581, wecom:437, weixin:493+527, whatsapp:259+500, imessage:326+379, feishu:282+387, task_executor.py:643, messaging/driver.py:714). None can consume the flag today — consume_needs_reinjection has exactly 1 consumer, chat_runner.py:7152 — so the general fix is genuinely larger; accepted-and-deferred, but the title "fix(context): restore skills after compaction" reads broader than the dashboard-only scope shipped.
  • Reuse is correct, not duplication: the only existing arming site, session_compaction.py:636, fires solely on SessionManager-initiated compaction (compact_if_needed), which none of these three dashboard paths traverse.

[FIRST-PRINCIPLES-REVIEWED] df70886

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants