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
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
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.
This branch has not been deployed
2 participants
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.