Skip to content

feat(track-a): opencode-luthien plugin integration - #759

Draft
PaoloC68 wants to merge 1 commit into
mainfrom
trajectory/track-a-pr-c-plugin
Draft

feat(track-a): opencode-luthien plugin integration#759
PaoloC68 wants to merge 1 commit into
mainfrom
trajectory/track-a-pr-c-plugin

Conversation

@PaoloC68

@PaoloC68 PaoloC68 commented May 21, 2026

Copy link
Copy Markdown
Collaborator

Part 3 of 3 splitting PR #614.

Depends on PR #758 (passthrough routes used by smoke test) — merge order: #757#758#759.

Changes

  • .gitmodules: Adds opencode-luthien submodule (URL: LuthienResearch/opencode-luthien)
  • plugins/README.md: Plugin installation instructions
  • README.md: Updated install section
  • dev-README.md: Smoke test section
  • scripts/track_a_smoke.sh: End-to-end smoke test (uses CLIENT_API_KEY per PR feat(track-a): multi-provider passthrough routes + 4 security fixes #758 auth model)

Notes

Closes part of #614.

@claude

claude Bot commented May 21, 2026

Copy link
Copy Markdown
Contributor

Review: PR #759 — opencode-luthien plugin integration

Thanks for splitting #614 into reviewable chunks. The intent is clear and the docs sketch is helpful, but several things here are broken as written and will block anyone trying to actually run the smoke test or merge cleanly.

Blockers

  1. scripts/track_a_smoke.sh cannot run successfully — multiple references to things that don't exist:

    • The script starts python -m tests.luthien_proxy.e2e_tests.mock_openai.server and ...mock_gemini.server, but only mock_anthropic exists in tests/luthien_proxy/e2e_tests/. There is no mock_openai/ or mock_gemini/ package.
    • Even mock_anthropic.server has no if __name__ == "__main__" entry point, so python -m tests.luthien_proxy.e2e_tests.mock_anthropic.server --port 9000 does nothing and silently exits.
    • The SQL query at the end (SELECT session_id, agent, model, endpoint FROM request_logs ...) references an agent column that doesn't exist in the schema (migrations/sqlite/008_add_request_logs_table.sql). The query will fail.
  2. plugins/README.md URL is wrong. It still points to https://github.com/PaoloC68/opencode-luthien even though the PR body explicitly says it was corrected to LuthienResearch/opencode-luthien. .gitmodules and the README are now inconsistent.

  3. dev-README.md references a nonexistent env var. The Prerequisites list says ".env file with PROXY_API_KEY set", but PROXY_API_KEY doesn't exist anywhere in the codebase. The script header comment and the codebase both use CLIENT_API_KEY (see src/luthien_proxy/config_fields.py:86). Pick one and use it consistently.

  4. README.md links to docs/plugin-header-contract.md which doesn't exist (docs/ has only architecture-visual.html, plans/, policies.md, screenshots/, standalone-container.md). Either add the contract doc or drop the link.

  5. Changelog fragment is missing required frontmatter. Per changelog.d/README.md, fragments need:

    ---
    category: Features
    pr: 759
    ---
    

    The current changelog.d/track-a-plugin.md is freeform markdown with a # title and bullets — scripts/compile_changelog.py likely won't parse it correctly.

Quality / robustness

  1. set -euo pipefail is undermined by || true on every meaningful command. The script will report success even when OpenCode crashes, the gateway never started, or the SQL query failed. Either drop || true and let real failures fail the script, or explicitly capture exit codes and assert post-conditions (status, expected log lines, expected DB rows).

  2. No actual verification. Right now the script just spawns processes, runs opencode run, and dumps logs/queries to a gitignored directory. The "What It Tests" table in dev-README.md lists expectations ("Request logged with session_id, agent, model", "x-goog-api-key injected", "Plugin warns, OpenCode routes direct") but nothing in the script checks any of them. As written it's an evidence-collection harness, not a smoke test — consider renaming or actually asserting.

  3. Port handling is hardcoded and fragile. MOCK_ANTHROPIC_PORT=9000, GATEWAY_PORT=8000, etc. will fail on dev boxes already using those ports. Pick free ports dynamically (the existing e2e fixtures already do this) or fail loudly with a clear message. The "Pick free ports" comment in the script is misleading since the ports are literally hardcoded.

  4. Race conditions on startup. sleep 2 then sleep 3 is the usual story — sometimes the gateway isn't ready, the test runs anyway, fails, and looks flaky. Poll the gateway health endpoint until ready instead.

  5. Cleanup doesn't wait. The trap fires kill ... and exits immediately, potentially leaving zombies if the processes are slow to die. Add wait after kill or use pkill -P $$ for the process group.

  6. DB path assumption. The script queries ~/.luthien/local.db directly, but if the user has DATABASE_URL set in .env, the gateway will write somewhere else and the query will return empty (silently, because of || true).

  7. Environment leakage. The script set -a; source .env; set +a plus export of many vars pollutes the parent shell if the script is sourced. Minor, but worth a comment that it's meant to be executed, not sourced.

Smaller things

  • The README.md shows npm install opencode-luthien as the install command. Is the package actually published to npm yet, or is install really "build the submodule and copy dist/index.js" as dev-README.md describes? Pick the one that works today.
  • plugins/README.md is sparse — at minimum it should match what dev-README.md already says about build-and-copy (bun run build && cp dist/index.js ~/.config/opencode/plugins/...).
  • Per CLAUDE.md "One PR = One Concern": this bundles submodule scaffolding + user-facing README change + dev smoke harness. They're related, but the README/user-facing copy could land independently of the smoke script. Not a hard blocker, just a heads-up.
  • The dev-README says the pytest variant is @pytest.mark.skip and documents the procedure only — that's fine, but consider linking from the script back to that test file so the connection is discoverable.

What's good

  • The PR is genuinely small and scoped (196 / 6 files), and the dependency / merge order is called out clearly in the body.
  • Good instinct to mark the pytest version skipped rather than wedging a binary dep into CI.
  • The scenario table in dev-README.md is a nice spec to assert against once verification is wired up.

Recommended next step

I'd hold this until at least the blockers are resolved — particularly the missing mock modules / missing __main__ / nonexistent agent column, since they mean the smoke script can't have been run end-to-end yet. Once those are fixed, the rest is polish.


Generated with Claude Code

@scottwofford

Copy link
Copy Markdown
Member

Claude-generated merge-queue triage of all open Luthien PRs, requested by Scott (Jul 7, 2026). Advisory only; Scott has not yet acted on these recommendations.

Recommendation: close, together with #757 and #758.

This depends entirely on #758's routes and migration; with that closing in favor of #796, the smoke test and docs here reference infrastructure that will not exist in this form (the May review also found the script references mock modules and a schema column that are not present). If OpenCode plugin support becomes a priority, it should be rebuilt against the passthrough shape that actually ships.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants