fix: preserve redacted thinking blocks during reasoning cleanup by tomolom · Pull Request #9 · cortexkit/magic-context

tomolom · 2026-04-11T05:31:42Z

Summary

stop stripClearedReasoning() from treating redacted_thinking as clearable reasoning content
add a regression test proving assistant redacted_thinking blocks are preserved unchanged
fixes Anthropic redacted_thinking blocks are stripped in stripClearedReasoning() #8

Testing

bun test packages/plugin/src/hooks/magic-context/strip-content.test.ts
bun run typecheck

Avoid stripping Anthropic redacted_thinking parts during stripClearedReasoning and add regression coverage for the preserved assistant content path. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>

Copilot

Pull request overview

This PR fixes an Anthropic compatibility issue where stripClearedReasoning() could remove redacted_thinking content blocks from assistant messages, which must be replayed unchanged.

Changes:

Stop treating redacted_thinking parts as clearable/strippable reasoning in stripClearedReasoning().
Add a regression test ensuring redacted_thinking parts are preserved unchanged during reasoning cleanup.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
`packages/plugin/src/hooks/magic-context/strip-content.ts`	Removes `redacted_thinking` from the set of part types eligible for cleared-reasoning stripping.
`packages/plugin/src/hooks/magic-context/strip-content.test.ts`	Adds a regression test asserting `redacted_thinking` parts are preserved when stripping cleared reasoning.

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

tomolom · 2026-04-11T05:37:36Z

I considered making this provider-gated, but I think the unconditional fix is the safer shape here.

Reasoning:

stripClearedReasoning() does not currently receive provider context.
More importantly, redacted_thinking is already the provider-specific discriminator. If that part type is present, it is the opaque block that must be preserved rather than treated like clearable thinking / reasoning text.
Leaving redacted_thinking in CLEARED_REASONING_TYPES means the cleanup path can delete a valid opaque block based only on missing thinking / text fields, which is wrong regardless of how provider detection is wired higher up.

If we want additional Anthropic-specific hardening later, that would make more sense one layer up in the transform pipeline (for example, skipping mutation of the latest assistant message when Anthropic reasoning blocks are present). But for this bug, removing redacted_thinking from the clearable-reasoning set seems like the correct local fix.

ualtinok · 2026-04-11T07:15:09Z

Thanks! I never encountered redacted_thinking tag myself. Good catch!

…fields Defense-in-depth layer on top of PR #9 (Tom's fix that removed redacted_thinking from CLEARED_REASONING_TYPES). The predicate inside stripClearedReasoning treated any thinking/reasoning part where both `thinking` and `text` fields were undefined as a cleared shell and dropped it. That's correct for legitimate cleared shells (which have both fields explicitly set to "[cleared]"), but wrong for edge-case shapes where a provider emits a thinking-type part carrying only non-standard fields like `data` or `signature`. Dropping such parts from the latest assistant message causes Anthropic to reject the request with 'thinking or redacted_thinking blocks in the latest assistant message cannot be modified'. This adds a 2-line guard: if neither `thinking` nor `text` key is present on the part, we cannot prove it is cleared, so we preserve it. Drop-callback-cleared parts are not affected because they set existing fields to "[cleared]" (keys remain present). No call sites change, no new state, no watermark, no cache-bust risk. Refs: #8, #9

Addresses the four findings I had initially triaged as "skip / defer" in the prior Athena audit but the user asked to fix all of them. Oracle re-verified each issue at an exact file:line before the fix. ### Finding #10 — dead upper clamp in deriveTriggerBudget `packages/plugin/src/hooks/magic-context/derive-budgets.ts` The `Math.min(executeThresholdPercentage, 100)` upper clamp was dead in production: every caller resolves the threshold through `resolveExecuteThreshold()` which already caps at `MAX_EXECUTE_THRESHOLD` (80). Removed the upper clamp while keeping the lower one (negative inputs still floor to 0). Added an intentional comment explaining the invariant so future audits don't re-flag it. Updated `derive-budgets.test.ts` so the previously misleading "clamp-to-100" assertion now documents the true contract. ### Finding #9 — Bun.hash in memory-injection test `packages/e2e-tests/tests/memory-injection.test.ts` The test reimplemented the plugin's project-identity and content-hash logic using `Bun.hash()`, coupling the test to a runtime-specific hash that Bun does not guarantee stable across versions. Replaced both usages with direct imports of the production helpers: - `resolveProjectIdentity(...)` from project-identity.ts - `computeNormalizedHash(...)` from normalize-hash.ts (stable MD5) Side-effect: the normalized-hash seed now matches what the plugin would actually compute when it writes the row itself, so the dedup path is exercised correctly. ### Finding #6 — doctor comment stripping on deprecated-key migration `packages/plugin/src/cli/doctor.ts` The migration for `experimental.compaction_markers` used `delete mcConfig.experimental` when the object became empty. With comment-json, deleting a key also drops any "before-property" comments anchored to it — including comments the user wrote above the whole `experimental:` section. Stopped auto-removing the empty object. The empty `experimental: {}` stays behind, which is cheap, preserves upstream comments, and is easy for users to clean up manually. Documented the constraint in an Intentional: comment so future audits don't re-flag it. ### Finding #5 — four e2e test assertion gaps `packages/e2e-tests/tests/` - smoke.test.ts: the body-contains assertion only proved the harness transported user text, not that the plugin ran. Added a `"Magic Context"` check (phrase lives in magic-context-prompt.ts and is stable across agent-prompt variants). Now fails if the plugin is disabled or its system-prompt injection regresses. - context-limits.test.ts: assertion was `35 <= pct <= 45` for a scenario where the plugin MUST record exactly 40. Tightened to `expect(pct).toBe(40)`. The old range let a 25% regression surface (e.g. falling back to a 128K default instead of the configured 50K custom limit) slip through. - short-context-overflow.test.ts: the `for (const i of 1..30)` loop silently swallowed prompt-send exceptions via `try { } catch {}`. A real overflow during the test would not fail it. Added `turnErrors[]` tracking with an explicit `expect(turnErrors).toEqual([])` that fails the test and prints per-turn error messages if any send throws. The overflow guard is now meaningful. - slow-historian.test.ts: the non-blocking invariant was a wall-clock `turn12Latency < 5000ms` assertion, which was flaky under CI load and didn't actually test the property that matters. Replaced with request-ordering: fire turn 12 without awaiting, then wait up to 3s for its request to appear at the mock. Historian has an 8s delay; if main were blocked on historian, the main-turn-12 request would not reach the mock until historian finished. Also asserts the historian in-flight count is unchanged at the moment main-turn-12 arrives — deterministic proof main is not blocked. Kept `MAIN_LATENCY_BUDGET_MS` as a constant for documentation. ### Verified - 499 plugin tests pass (was 486; +13 from new derive-budgets tests for `resolveHistorianContextLimit` added in the prior commit). - 13 e2e tests pass under the configured 120s timeout. - Typecheck clean on both packages. ### Not included in this commit Uncommitted changes to `nudge-injection.ts` and `nudge-injection.test.ts` from a prior session (thinking-bearing assistant message protection) are left untouched — they are unrelated to the council findings and deserve their own review and commit.

…busts Historian publication, recomp, and partial-recomp all call clearInjectionCache(sessionId) after promoting new compartments/facts so the next transform rebuilds <session-history>. But previously they did NOT mark the session as flushed. If the next transform happened to be a defer pass (scheduler=defer, no explicit flush), prepareCompartmentInjection would recompute from scratch with new compartments -> DIFFERENT rendered message[0] -> silent provider cache bust on a pass with no legitimate cache-bust trigger. Add onInjectionCacheCleared callback to CompartmentRunnerDeps. Runners invoke it after every cache invalidation, and callers (transform.ts and hook.ts's /ctx-recomp path) register it to call flushedSessions.add(sid). This ensures the next transform is treated as cache-busting, which is accurate — we ARE changing message[0] — rather than silently accepting the bust on a defer pass. Closes council Finding #9 (HIGH, Opus + GPT 5.4 high).

Two definitions of 'cache-busting' coexist: - system-prompt-hash.ts + inject-compartments.ts: flush-only - transform-postprocess-phase.ts: flush-OR-execute Intentional by design but undocumented — a maintenance footgun. Add detailed design comments at both definition sites explaining why the asymmetry matters: - Adjunct state (docs, user profile, sticky date) is disk/config- derived and unrelated to pending ops. Flush-only ensures it refreshes only on explicit user-driven events. - Message-level mutations (pending ops, sentinel registration, tool-drop finalization) correctly fire on scheduler 'execute' passes because that's when queued user drops get materialized. Historian publication bridges the two via flushedSessions.add (just fixed in the previous commit, council Finding #9). No behavioral change. Closes council Finding #12 (MEDIUM, 4 members).

) Council audit finding #9 (8/9 members): the doctor --issue output sanitizer only stripped paths and usernames, so any log line carrying an API token, AWS key, GitHub PAT, OpenAI key, etc. would land verbatim in the user-shareable issue report. This expands packages/plugin/src/cli/logs.ts `sanitizeLogContent` with a structured `SECRET_PATTERNS` table covering: - Anthropic API keys: `sk-ant-api03-*`, `sk-ant-*` - OpenAI API keys: `sk-proj-*`, `sk-*` (legacy) - GitHub fine-grained PATs (`github_pat_*`) and classic tokens (`gho_/ghp_/ghs_/ghu_/ghr_`) - HuggingFace tokens (`hf_*`) - AWS access keys (`AKIA*`, `ASIA*`) and secret keys in assignment context - Slack tokens (`xox[abprsuvc]-*`) - Google API keys (`AIza*` 39-char total) - Generic shell-style env var assignments where the variable name contains `API_KEY`, `TOKEN`, `SECRET`, `PASSWORD`, `CREDENTIAL`, or `PRIVATE_KEY` — keeps the variable name visible so the report is still useful for debugging - JSON-style assignments for `api_key`, `access_token`, `auth_token`, `bearer_token`, `client_secret`, `password`, `private_key`, `secret_key` — case-insensitive, both spellings (underscore/hyphen) - HTTP `Authorization: Bearer <token>` headers - JWTs (3-segment, requires `eyJ` prefix to avoid base64 false positives) Each pattern is intentionally narrow so we under-redact rather than over-redact: model identifiers like `anthropic/claude-haiku-4-5` and non-secret env vars like `OPENCODE_VERSION=1.4.0` pass through unchanged. The pattern table is ordered so that more specific provider shapes win over the generic env-var fallback, preserving token type information in the redacted output (e.g. `<AWS_ACCESS_KEY_ID_REDACTED>` rather than just `<REDACTED>`). Adds 39 unit tests covering every secret-pattern variant plus combined sanitization scenarios that exercise both phases of the sanitizer together (paths + tokens in the same line, multiline log content, case-insensitive matching, false-positive resistance for legitimate content).

@latest

Three logical groups of fixes bundled together. ## A. Hide subagent prompts from TUI render (issue #50 part 2) Historian, dreamer, sidekick, compressor, and user-memory subagent calls were sending their full system prompt body as a regular `text` part on the user message. OpenCode TUI rendered this as a giant unreadable "user message" in the subagent pane (compaction prompts can be >90K chars). Fix: add `synthetic: true` to the prompt text part on all 7 subagent call sites. Verified via OpenCode source: - `message-v2.ts:74` (toModelMessagesEffect) only filters `!part.ignored` for user-role text, so synthetic still reaches the LLM. - `transcript.ts:85` filters `!part.synthetic` for TUI render, hiding the prompt body from the visible pane. - OpenCode's own `compaction.ts:547` and `prompt.ts:234-241` use the same pattern — the LLM must read these prompts to function. E2E proof: `historian-success.test.ts` still passes (29/29 e2e tests pass) — if synthetic filtered the prompt out, historian would receive empty input and produce no compartment; the test asserts a compartment is published. ## B. Audit findings — 8 confirmed real bugs + 2 false-positive comments Council audit of the post-v0.16.1 stack identified 10 issues. Manual re-verification against source confirmed 8 real bugs and 2 false positives. ### Real bugs 1. `setup-opencode.ts` cast `existing.plugin as string[]` and called `.startsWith()` on each entry, throwing TypeError on tuple-form `["@pkg/name", { ...options }]` entries. Now imports `isDevPathPluginEntry` and `matchesPluginEntry` from the shared adapter and operates on the raw array. Tuples preserved on write, dev-path entries detected (no double-add) but never replaced. 2. `doctor-opencode.ts` filtered the plugin array with `(p): p is string => typeof p === "string"` BEFORE writing it back, silently destroying every tuple entry on every doctor run. Same fix: operate on the raw array. When upgrading a pinned tuple entry to @latest, the options object is preserved by mutating only index [0] of the tuple. Same fix applied to TUI config plugin handling. Also fixes finding #9: the inline `isDevPathEntry` only matched `opencode-magic-context`, missing bare `magic-context` paths (post-rename) that the shared helper handles correctly. 3. `paths.ts::getOpenCodePluginCacheDir` had a Windows branch resolving to `%LOCALAPPDATA%/opencode/Cache/packages` while OpenCode (and our `data-path.ts` plugin helper) actually use `~/.cache/opencode/packages` on every platform via the `xdg-basedir` fallback. Removed the Windows-specific branch so `doctor --force` clears the right directory. This regressed the exact same fix already applied to `data-path.ts`. 4. `dashboard-release.yml` deploy-updater used `publish_dir: .` + `force_orphan: true` + `keep_files: false` together with an invalid `include_files` input that peaceiris/actions-gh-pages does not support. Result: every dashboard release published the entire repo checkout to gh-pages with a single orphan commit, wiping history. Fixed by staging just `latest.json` in `_updater_publish/`, pointing `publish_dir` at that dir, setting `keep_files: true` and `force_orphan: false`. 5. `dashboard-release.yml` used a hardcoded `sleep 30` before downloading `latest.json` from the draft release. First-time asset uploads can take longer than 30s, causing silent failures. Replaced with a 20×15s retry loop (5 minutes total). 8. `release.yml` `github-release` job depended only on test jobs, not on the three publish jobs, so a release page could be created on GitHub while one of the @cortexkit/* npm packages was missing. Now `needs: [..., publish-npm, publish-npm-pi, publish-npm-cli]`. 10. `doctor-pi.ts` had `add(results, "pass", "No known conflicting Pi extensions detected")` hardcoded with no detection logic. Replaced with real check for multiple magic-context entries in Pi `packages[]` (a real self-conflict that loads the plugin twice). ### False positives — documented inline 6. `setup-opencode.ts` Step 8 (OMO conflict prompt) is gated by `!hadExistingSetup`. Audit flagged this as "skipped for existing users", but existing users hit the same OMO conflict-fix logic via the `hadExistingSetup` branch at lines 231-257 (which calls `detectConflicts` + `fixConflicts` covering all OMO hooks). Added inline comment so future audits don't re-flag. 7. `migrate.ts` reports `messageCount: entries.length - 2`. Audit flagged this as off-by-N. The `- 2` correctly subtracts the structural `session` + `model_change` entries that lead every Pi JSONL file; the result counts boundary marker + all migrated message entries + (when present) compaction marker — exactly what "migrated entries" means in CLI output. Added inline comment. ## C. Discord URL update Switched README Discord badge + nav link from `discord.gg/F2uWxjGnU` to `discord.gg/DSa65w8wuf`. ## Verification - typecheck: 0 errors across plugin + pi-plugin + cli - lint: 0 errors (after biome autofix) - plugin tests: 1050 / 1050 pass - pi-plugin tests: 236 / 236 pass - cli tests: 58 / 58 pass - e2e tests: 29 / 29 pass (proves synthetic:true reaches LLM) - manual smoke tests: - tuple-form other plugin preserved when adding magic-context: ✓ - pinned tuple of magic-context upgraded to @latest, options preserved (`myOption` survives `0.16.0 → @latest`): ✓ - dev-path entry detected (hasDev=true), not double-added: ✓ - bare `magic-context` (post-rename) dev path detected: ✓ ## Reusable helpers `isDevPathPluginEntry` and `matchesPluginEntry` were promoted from private to exported in `packages/cli/src/adapters/opencode.ts` so both `setup-opencode.ts` and `doctor-opencode.ts` can use the same source of truth for plugin-entry classification — eliminates the drift that caused finding #9.

Copilot AI review requested due to automatic review settings April 11, 2026 05:31

Copilot started reviewing on behalf of tomolom April 11, 2026 05:32 View session

Copilot AI reviewed Apr 11, 2026

View reviewed changes

ualtinok merged commit f8dea45 into cortexkit:master Apr 11, 2026
5 of 6 checks passed

tomolom deleted the fix/redacted-thinking-strip branch April 11, 2026 07:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: preserve redacted thinking blocks during reasoning cleanup#9

fix: preserve redacted thinking blocks during reasoning cleanup#9
ualtinok merged 1 commit intocortexkit:masterfrom
tomolom:fix/redacted-thinking-strip

tomolom commented Apr 11, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

tomolom commented Apr 11, 2026

Uh oh!

Uh oh!

ualtinok commented Apr 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

tomolom commented Apr 11, 2026

Summary

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

tomolom commented Apr 11, 2026

Uh oh!

Uh oh!

ualtinok commented Apr 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants