fix(opencode): persist synthetic context outside user messages - #524
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughOpenCode now inserts SessionStart and workflow-state context as deterministic synthetic text parts. Shared helpers validate persisted identities, preserve ordinary user text, and keep part ordering stable. Plugins and tests now use the persisted-part flow. ChangesOpenCode context persistence
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant SessionStart
participant WorkflowState
participant contextVisibility
SessionStart->>contextVisibility: insert sessionStart synthetic part
WorkflowState->>contextVisibility: find ordinary user text
WorkflowState->>contextVisibility: insert workflowState synthetic part
contextVisibility->>SessionStart: return ordered persisted part
contextVisibility->>WorkflowState: preserve ordinary user part
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.opencode/lib/context-visibility.js (1)
42-44: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winOrdinal guard checks one slot instead of both, in both identical helper copies. With
sourceOrdinal === 2,workflowStatesucceeds andsessionStartthrows, so the turn persists one context slot. The contract requires a throw when the ordinal cannot reserve both slots.
.opencode/lib/context-visibility.js#L42-L44: comparesourceOrdinalagainst the maximum offset of allCONTEXT_PART_KINDSentries.packages/cli/src/templates/opencode/lib/context-visibility.js#L42-L44: apply the identical change to preserve byte parity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.opencode/lib/context-visibility.js around lines 42 - 44, The ordinal guard in both copies of the context-visibility helper must validate against the maximum offset among all CONTEXT_PART_KINDS entries, ensuring an ordinal can reserve both context slots before proceeding. Update .opencode/lib/context-visibility.js lines 42-44 and packages/cli/src/templates/opencode/lib/context-visibility.js lines 42-44 identically to preserve byte parity.packages/cli/test/templates/opencode.test.ts (1)
616-618: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCompare IDs by code units, not by locale collation.
localeCompareapplies locale collation.insertSyntheticTextPartorders IDs with the code-unit operators<and>, and stored replay sorts bytes. Real OpenCode IDs mix uppercase and lowercase base62 characters, where the two orders differ. The current fixtures use lowercase suffixes only, so this test can pass while byte-order replay differs.Use the same comparator as the implementation. The same change applies at lines 1013-1022.
♻️ Proposed fix
expect(parts.map(part => part.id)).toEqual( - [...parts].sort((left, right) => left.id.localeCompare(right.id)).map(part => part.id), + [...parts] + .sort((left, right) => + left.id < right.id ? -1 : left.id > right.id ? 1 : 0, + ) + .map(part => part.id), );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/test/templates/opencode.test.ts` around lines 616 - 618, Update the ID sorting assertions in the test, including the comparable assertion around the second referenced range, to use code-unit ordering with the same direct comparison semantics as insertSyntheticTextPart and replay sorting instead of localeCompare. Preserve the existing mapped ID expectations while ensuring mixed-case base62 IDs are ordered consistently with the implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.opencode/lib/context-visibility.js:
- Around line 42-44: The ordinal guard in both copies of the context-visibility
helper must validate against the maximum offset among all CONTEXT_PART_KINDS
entries, ensuring an ordinal can reserve both context slots before proceeding.
Update .opencode/lib/context-visibility.js lines 42-44 and
packages/cli/src/templates/opencode/lib/context-visibility.js lines 42-44
identically to preserve byte parity.
In `@packages/cli/test/templates/opencode.test.ts`:
- Around line 616-618: Update the ID sorting assertions in the test, including
the comparable assertion around the second referenced range, to use code-unit
ordering with the same direct comparison semantics as insertSyntheticTextPart
and replay sorting instead of localeCompare. Preserve the existing mapped ID
expectations while ensuring mixed-case base62 IDs are ordered consistently with
the implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3cd4c163-9b63-410b-b441-38afc738b333
📒 Files selected for processing (9)
.opencode/lib/context-visibility.js.opencode/plugins/inject-workflow-state.js.opencode/plugins/session-start.js.trellis/spec/cli/backend/workflow-state-contract.mdpackages/cli/src/templates/opencode/lib/context-visibility.jspackages/cli/src/templates/opencode/plugins/inject-workflow-state.jspackages/cli/src/templates/opencode/plugins/session-start.jspackages/cli/test/regression.test.tspackages/cli/test/templates/opencode.test.ts
|
Reviewed this in depth. The premise holds and I verified it against OpenCode 1.17.18's actual implementation rather than against the description — extracted the embedded JS from the installed binary and checked a live
It also rebases onto current Three things I'd like before merge. Two are missing tests that let real mutations survive; I ran the mutations rather than eyeballing coverage. 1. The "identity source must be the minimum part" invariant is undefended. I inverted the comparator in Why it matters, with real ID shapes from the live DB — text
Min-selection is what makes the scheme safe. Could you add a case with a text part and a file part asserting both synthetic IDs sort below both ordinary parts? 2. The ID-format strictness is also untested, and drift is not hypothetical. Loosening In the live DB, 2435/2440 IDs match the current pattern and 5 use an older 19-char format ( Worth noting the pattern is stricter than the algorithm needs: only the leading 12 hex participate in ordering, so a tail-length change alone would be harmless to correctness yet currently kills injection. A test using that real legacy ID, asserting user parts survive untouched, would pin the intended behavior either way. 3. The
The new test fires (For what it's worth, I checked the two failure modes I was worried about and both are fine: double-injection after compaction cannot happen, and permanent suppression from a false-positive dedupe is not realistic, since a Optional, cheap: A few things I checked and found clean, so you don't need to re-litigate them: Test quality is otherwise strong — I ran 12 mutations and 10 were killed, several by many tests at once, and the One thing I could not determine: whether the desktop/web client renders synthetic user text parts inside the message bubble. I confirmed they are filtered out of prompt reconstruction, timeline, fork, copy and revert. Cosmetic either way, and no worse than today's inline concatenation. |
…andler Addresses the three review items on mindfold-ai#524. The `session.compacted` handler cannot work. It clears the in-memory processed flag, but `chat.message` then calls `hasPersistedInjectedContext`, which reads the whole session with no compaction filter and still finds the pre-compaction marker — so the clear is undone on the very next line. Removed it, and reworked the test that fired the event so it exercises the live path (a fresh process meeting an already-marked session) instead of certifying dead code. Re-injecting context after compaction is now recorded in the spec as a known gap rather than implied to work. Two mutations survived the suite before this change: - Inverting the comparator in `findIdentitySourcePart` killed zero tests; no fixture had two ordinary parts in one message. Added a text-part plus file-part case using real ID shapes from a live DB. - Loosening `PART_ID_PATTERN` also killed zero tests; the only coverage was a part with no `id` at all, which fails an earlier guard. Added a case using the 19-character legacy format OpenCode still has in the wild, asserting the user's parts are left untouched. The obvious assertion for the first one — both synthetic IDs sort below both ordinary IDs — passes with the comparator inverted, because the deciding byte becomes a digit against a letter. It now pins the exact ordinal prefixes that only minimum-sourced selection produces.
Brings 18 commits from main, including 0.6.15, the DSH minimal adapter (#547), and the marker-label routing fix. Beta stays on 0.7.0-beta.3 and keeps its own submodule pointers. Git merged three files cleanly whose *content* was contradictory, so those are the ones worth re-reviewing: - workflow.md: main puts dsh in the inline-execution group (#547 has no sub-agent surface), beta puts it in the class-2 sub-agent group (#548 added one). Both edits applied, leaving dsh in both groups. Resolved to beta's semantics — dsh removed from the inline markers in the template, the dogfood copy and the marketplace mirror. - dsh was registered twice: once by #547 on main and once by #548 on beta, in cli/index.ts, commands/init.ts and configurators/index.ts. Different lines each time, so no conflict was raised; tsc caught it. Deduped, keeping beta's wording. - opencode.test.ts: beta's layered-workflow tests fed the plugin a bare {type,text} part, which #524 on main no longer accepts — insertSyntheticTextPart requires an ordinary part with a persisted identity and now emits the breadcrumb as its own part instead of rewriting the user's text. The tests asserted on parts[0] and silently saw the unmodified prompt. Rewritten against the synthetic-part contract; they still verify the layered resolution they were written for. Declared conflicts resolved to beta for everything dsh-related and for the version files; workflow_phase.py took main's _PLATFORM_MARKER_LABELS table, which subsumes beta's dsh-only alias. Verified on the merged tree rather than assumed: - core 344 passed, cli 1825 passed, lint and tsc clean - all 22 platforms resolve a non-empty routing block - dsh resolves the sub-agent route, not the inline one - built the CLI and ran `init --dsh`: 86 files, with the three trellis-agent-* role skills beta expects - marketplace mirror byte-identical to the bundled template
2 participants
Summary
Verification
pnpm install --frozen-lockfilepnpm typecheckpnpm --filter @mindfoldhq/trellis lint:py(0 errors; existing unused-import warnings only)pnpm lintpnpm buildpnpm test(Core: 333 passed, 1 skipped; CLI: 1645 passed; OpenCode: 70 passed)git diff --checknode --checkfor all changed JavaScript filesRisk
The identity helper intentionally accepts the current persisted OpenCode
prt_ID format. Unsupported or incomplete identities fail closed: plugins log the error and preserve all ordinary input parts unchanged. Regression coverage includes both plugin orders, attachment-only turns, duplicate refusal, persisted-history dedupe, compaction, and ID-sorted replay.Rollback
Revert commit
ac66e207; this restores the previous in-place context injection behavior without a data migration.Summary by CodeRabbit
New Features
Bug Fixes
Tests