Repository navigation
Conversation
Dependency ReviewThe following issues were found:
License Issues.github/workflows/latest-hosts.yml
OpenSSF Scorecard
Scanned Files
|
Memory benchmarkCompared base
Interpretation: Differences larger than their measured MAD: Stable RSS -1.1 MiB ± 0.9 MiB, Physical peak -2.2 MiB ± 1.6 MiB. Extension overhead above pi baseline
Values are medians of 6 alternating, paired runs. The value after Environment: pi
|
patlux
left a comment
There was a problem hiding this comment.
The contract test is a genuinely useful addition — it found a real OMP bug and covers all three wires on both hosts. The one thing I'd block on is the Pi pin version; the rest are comments/nits.
| - name: Install pi | ||
| run: | | ||
| npm install -g @earendil-works/pi-coding-agent@latest | ||
| npm install -g @earendil-works/pi-coding-agent@1.1.0 |
There was a problem hiding this comment.
Pinning Pi to 1.1.0 is a coverage regression: 1.2.0 (2026-10-12) and 1.3.0 (2026-10-15, published before this PR was opened) are out, and CI previously tested @latest. A PR that breaks on 1.3.0 now merges green and only shows up in the weekly job, after the fact.
Suggest:
- pin to the current latest (1.3.0) and make "bump the pins" part of the release checklist, or
- keep a pinned job as the required gate and add a
latestjob withcontinue-on-error: truein PR CI, so drift is visible on the PR rather than a week later.
Either way, CONTRIBUTING should say who bumps the pin and when (a green Latest host compatibility run should be the trigger).
| - name: Record versions | ||
| run: | | ||
| { "$PI_BIN" --version; "$OMP_BIN" --version; node --version; bun --version; } | tee -a "$GITHUB_STEP_SUMMARY" | ||
| - run: npm test |
There was a problem hiding this comment.
On its own this job fails silently into the Actions tab (no issue, no notification beyond the default email). Fine if #163 lands; if this merges without it, note in CONTRIBUTING that someone has to look at the weekly run.
| const root = fileURLToPath(new URL("../", import.meta.url)) | ||
| const extension = resolve(process.env.COMMANDCODE_TEST_PACKAGE_DIR ?? root, "index.ts") | ||
| const fixture = join(root, "tests/fixtures/protocol-tools-extension.ts") | ||
| const models = [ |
There was a problem hiding this comment.
These three ids encode assumptions about the static catalog: deepseek/deepseek-v4.1-flash must be reasoning: true in commandcode-catalog.ts for the reasoning_content replay assert, and claude-sonnet-4-6 must resolve to Anthropic via apiForModelId. A catalog refresh that changes either will fail this test with an unrelated-looking assertion deep in the mock server. Add a comment stating the dependencies, or assert them up front with a clear message.
| supportsStore: false, | ||
| supportsDeveloperRole: false, | ||
| supportsReasoningEffort: MODEL_EFFORTS[model.id] !== undefined, | ||
| replayReasoningContent: model.reasoning, |
There was a problem hiding this comment.
This flag is now also sent to Pi. The comment says Pi ignores it — a link to the OMP flag definition (or a one-line note on which OMP version introduced it) would make the "OMP requires explicit opt-in" claim verifiable later when either host changes behavior.
|
|
||
| ## Unreleased | ||
|
|
||
| - Preserve Chat Completions reasoning history on OMP with an explicit replay compatibility flag, verified by real Pi/OMP follow-up requests rather than only visible thinking deltas. |
There was a problem hiding this comment.
"verified by real Pi/OMP follow-up requests rather than only visible thinking deltas" is verification detail for the PR, not a user-facing note. Suggest: "Preserve Chat Completions reasoning history on OMP; previous reasoning was dropped from follow-up requests."
pierreraby
left a comment
There was a problem hiding this comment.
Approved, with one P1 test-hardening note below that I deliberately leave non-blocking. Verified locally at the exact head in an isolated worktree (npm ci): tsc --noEmit PASS, test:host-protocols 6/6 (pi + omp x chat/responses/messages, incl. reasoning replay, parallel tool pairing, and the #141 call_id <= 64 pin on the Responses wire), full test:unit exit 0 (401 checks), Prettier + git diff --check PASS. I also had this head independently reviewed in a fresh context: the OMP fix itself is judged correct and minimal (one compat flag confined to the Chat Completions branch, Pi unaffected, no reach into generate/diagnostics), contracts largely non-vacuous, CI pinning + weekly @latest separation sane, no routing regression. I closed the reviewer's committed-range check myself (git diff --stat 3e43a7c..HEAD: 10 files, 2 commits, all consistent with the description).
P1 (test-only, non-blocking): the "image results on follow-up" assertion (test-host-protocols.mjs:88, assert.match(history, /image|image_url/)) is vacuous — the follow-up history always contains the tool-call arguments ({"image":"A"}), so the regex matches even if image tool results were dropped. One-line fix suggested: assert the payload instead, e.g. /data:image\/png;base64,iVBORw0KGgo|"type":"image"|image_url/. I approve because the shipped fix is solid and image forwarding is already pinned non-vacuously elsewhere (test-pi-local provider path) — but please land that line with or right after this PR since it is the PR's own acceptance criterion.
Carry the OMP Chat Completions reasoning replay fix and real-host protocol contracts from patlux's d690621, adapted to the current main entrypoint types. Exercise Pi and OMP over Chat Completions, Responses and Anthropic Messages, with structural system/image assertions, tool-result pairing, tested-package catalog preflight and bounded subprocess cleanup. Wire the contracts into npm, the OMP CI job and installed-package smoke tests without old host pins or weekly automation. Carries #162 Co-authored-by: Patrick Wozniak <email@patwoz.de>
|
Carried forward, integrated and released in v0.8.0. Thank you @patlux for the original contribution. It was adapted to current main through #200; your contribution is credited in the carry/merge history and the released changelog. The OMP reasoning-replay fix and six real-host protocol contracts (Pi/OMP × Chat Completions/Responses/Anthropic Messages) are integrated, including actual image-payload assertions and installed-package coverage. This was an adapted carry, not the entire original diff: the old host pins, separate weekly latest-host workflow, stacked-base CI change and live multi-model extension were deliberately not adopted. Required current @latest host tests and the real-Pi declaration gate remain in place. The released npm tarball passed the required real Pi and OMP mock-API suites, including registry-package verification: release workflow. These are isolated mock tests, not live account, billing or performance verification. This original PR is superseded by the integrated carry, not abandoned. It is being closed without merging its old branch again. No source branch or linked issue is being deleted/closed as part of this cleanup. |
Summary
replayReasoningContentcompatibility flag. Verified failure before the flag and success after.Verification
npm test, both real hosts required: PASS.No npm release or merge is part of this PR. Independent of the routing/diagnostics stack; resolve the small
compatForModeloverlap when combining them.