fix(types): handle reasoning.encrypted content block type from Grok models#1732
Draft
potb wants to merge 7 commits intocode-yeongyu:devfrom
Draft
fix(types): handle reasoning.encrypted content block type from Grok models#1732potb wants to merge 7 commits intocode-yeongyu:devfrom
potb wants to merge 7 commits intocode-yeongyu:devfrom
Conversation
There was a problem hiding this comment.
3 issues found across 14 files
Confidence score: 4/5
- This PR looks generally safe to merge, but there is a user-facing risk:
reasoning.encryptedparts are filtered out before formatting insrc/tools/background-task/full-session-format.tsandsrc/tools/background-task/modules/formatters.ts, so the new handling is unreachable and content gets dropped even whenincludeThinkingis true. - The test in
src/hooks/thinking-block-validator/reasoning-encrypted.test.tsis tautological and doesn’t exercise production logic, so it won’t catch regressions in the validator. - Pay close attention to
src/tools/background-task/full-session-format.ts,src/tools/background-task/modules/formatters.ts,src/hooks/thinking-block-validator/reasoning-encrypted.test.ts- reasoning.encrypted handling is dropped and test coverage is ineffective.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/hooks/thinking-block-validator/reasoning-encrypted.test.ts">
<violation number="1" location="src/hooks/thinking-block-validator/reasoning-encrypted.test.ts:5">
P2: Test is tautological: it only checks a locally defined helper instead of exercising production logic, so it provides no real coverage of the thinking-block validator.</violation>
</file>
<file name="src/tools/background-task/full-session-format.ts">
<violation number="1" location="src/tools/background-task/full-session-format.ts:138">
P2: `reasoning.encrypted` parts are filtered out before formatting, so the newly added handling is unreachable and the content is discarded.</violation>
</file>
<file name="src/tools/background-task/modules/formatters.ts">
<violation number="1" location="src/tools/background-task/modules/formatters.ts:305">
P2: `reasoning.encrypted` parts are filtered out before formatting, so the newly added handling is unreachable and these parts are dropped even when `includeThinking` is true.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
4aec122 to
d45b87f
Compare
…NKING_TYPES Set Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <[email protected]>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <[email protected]>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <[email protected]>
…traction Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <[email protected]>
…g content extraction Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus <[email protected]>
d45b87f to
f634150
Compare
The filter that normalizes message parts before formatting only checked for 'thinking' and 'reasoning', so reasoning.encrypted parts were silently dropped even when includeThinking was true.
Contributor
Author
|
@cubic-dev-ai recheck |
@potb I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 13 files
Confidence score: 5/5
- Low-severity test quality concern only; no concrete user-facing regression is indicated, so this looks safe to merge.
src/hooks/thinking-block-validator/reasoning-encrypted.test.tsvalidates a local helper instead of production logic, so the test could pass even if the real thinking-family check is wrong.- Pay close attention to
src/hooks/thinking-block-validator/reasoning-encrypted.test.ts- test may not exercise actual production behavior.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/hooks/thinking-block-validator/reasoning-encrypted.test.ts">
<violation number="1" location="src/hooks/thinking-block-validator/reasoning-encrypted.test.ts:5">
P3: This test defines and validates a local helper instead of exercising production logic, so it will pass even if the real thinking-family check is incorrect (e.g., `startsWithThinkingBlock` still ignores `reasoning.encrypted`). It doesn’t provide regression protection for the intended change.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…thThinkingBlock and findPreviousThinkingContent Address review feedback (identified by cubic): the thinking-block-validator hook was missing reasoning.encrypted in two internal functions, and the test was tautological (testing a local helper instead of production logic). - Add reasoning.encrypted to startsWithThinkingBlock so messages starting with encrypted reasoning are recognized as having a thinking block - Add reasoning.encrypted to findPreviousThinkingContent so encrypted reasoning from prior turns can be recovered - Rewrite test to exercise createThinkingBlockValidatorHook directly with message fixtures instead of a locally defined helper
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1588
Grok models return
reasoning.encryptedcontent blocks. We only check for"reasoning"and"thinking", so these blocks get silently dropped during validation, extraction, and formatting.Added
reasoning.encryptedto theThinkingPartTypeunion andTHINKING_TYPESset, then updated all downstream checks. Content extraction guards on&& part.textso encrypted blocks without text are safely skipped.