Add token passthrough feature - #12
Conversation
7045a5f to
2e1e79a
Compare
dIvYaNshhh
left a comment
There was a problem hiding this comment.
The ctypes marshalling in here is careful work — _AgentOptionsStorage gets the buffer-lifetime problem right in a way that's easy to get wrong, _bind_optional_agent_options_symbol is the per-symbol try/except that #10's big shared block should have been, and _missing_options_symbol_message() is exactly the actionable error I was asking for on that PR. Good stuff.
Three things I think block merge:
1. The 12.9 MB of committed binaries. create_by_url_gateway (8.76 MB) and create_by_url_server (4.14 MB) are checked in as executables. I pulled the magic bytes — they're Mach-O 64-bit x86_64, so they won't run on Linux CI and need Rosetta on Apple Silicon, which makes create_by_url_run.sh unrunnable for most of the team. There's also no source or build recipe in the repo, so there's no way to review what they actually do — that's a supply-chain question I can't sign off on.
Could these be built from third_party/gopher-orch in the script, or pulled as release assets? Once they're in history they're there permanently.
2. The native-library search reorder (inline below). It ships to every PyPI user, not just developers, and turns "process happens to run from a writable directory" into arbitrary native code execution.
3. Empty access_token produces Authorization: Bearer , and the shipped example hits that path by default.
Plus a question I couldn't answer from the diff: whether the native side deep-copies the options struct at creation or retains the pointer. If it retains it, the storage is freed too early. Details inline.
Smaller stuff: Optional[object] defeats mypy on every new signature, runtime options get normalised at two different layers depending on which factory you use, and ffi/library.py now imports the public config layer. All noted inline.
Base is iml_new_apis, so this can't land until #10 does — and it shouldn't land alone regardless, because of the --remote submodule change (#13 reverts it).
bb91598 to
a46cfc5
Compare
dIvYaNshhh
left a comment
There was a problem hiding this comment.
Re-reviewed at a46cfc58. Eight of nine addressed, and a few went past what I asked. Resolving those threads.
Confirmed fixed:
-
Search order (
2c21fa03) — installed platform package back at position 1, cwd dev paths after it, and the comment now points atGOPHER_MCP_PYTHON_LIBRARY_PATH. Hijack vector closed. -
Empty
access_token(fc5ad5b0) — fixed in both places:access_token == ""normalises toNone, and_normalize_headersgates onif access_token:. Covered by a test. -
Options struct lifetime (
229ad3ff) — you went and got the answer to the question I couldn't resolve from the diff, and put it where the next reader will find it:Native BuildAgentOptions deep-copies this struct into C++ strings/maps during creation, so this storage only needs call-duration lifetime.
That, plus the comment explaining why
_bytesis pinned, is exactly right. Thanks for chasing the C side rather than just asserting it. -
Normalisation at two layers (
c012d442) — done once at the agent boundary now, so both factory paths fail at the same point. -
Layering (
28764f27) — the newgopher_mcp_python/runtime_options.pyactually breaks theffi → configdependency instead of papering over it. -
Typing —
RuntimeOptionsInputreplacesOptional[object]; zero occurrences left inagent.py, so mypy can see through it. -
Submodule pinning (
6f88270e) —GOPHER_ORCH_TRACK_REMOTE=1opt-in, pinned by default. -
Temp dir (
c75d5e8c).
Suite at the tip of the stack: 151 passed, 116 skipped, 3 failed — all three failures reproduce identically on origin/main (they need ./build.sh), so nothing regressed.
One item left, and it's the one I'd hold merge on: the 12.9 MB of committed binaries — no commit touches them. Detail inline.
Separately: the description is still empty. With a new public module, a new FFI struct, and a build-script behaviour change in here, a few lines of scope would help the next reviewer.
dIvYaNshhh
left a comment
There was a problem hiding this comment.
de69016c closes the last item — resolving the thread. Nothing outstanding from me on this PR now.
You took option 1 and did it properly:
- Both binaries deleted, and
create_by_url_run.shnow buildsheader_access_token_create_by_url_{server,gateway}fromthird_party/gopher-orchviacmake --build. build_header_binaries()only runs when the binaries are absent, so repeat runs don't pay for a rebuild.- It fails with an actionable message when the submodule isn't initialized, and auto-inits the nested
gopher-mcpwhen that's what's missing. - Core count falls back
sysctl -n hw.ncpu->nproc->4, so it works on macOS and Linux.
Net effect: the script now runs on Linux and Apple Silicon, where before it couldn't run at all.
Suite at the tip of the stack: 153 passed, 116 skipped, 3 failed — the three failures reproduce identically on origin/main (they need ./build.sh), so nothing regressed.
One thing for whoever clicks merge — not a code change. Deleting the files fixes the tree at HEAD, but the blobs are still reachable in this branch's history:
$ git rev-list --objects feature_token_protect --not origin/main | ...
4.0 MB examples/header/create_by_url_server
8.4 MB examples/header/create_by_url_gateway
added in e0c4bc47, removed in de69016c. The repo currently has allow_merge_commit, allow_squash_merge and allow_rebase_merge all enabled — and with a merge commit or rebase-merge those 12.4 MB land in main's history permanently, even though no commit on main would have them in its tree.
Squash-merging this PR avoids it entirely (the squash commit carries only the final tree). Worth agreeing before merge — cheap now, needs a history rewrite later. Same applies to #13 and #14, which sit on this branch.
Add ctypes bindings for gopher_orch_agent_options_t and optional *_with_options agent creation symbols. Route non-empty runtime options through the new native symbols and fail clearly when the loaded native library is too old. Cover options struct conversion, fallback behavior without runtime options, and missing-symbol errors with focused tests.
Forward configured runtime options into API-key and JSON native agent creation. Add runtime_options parameters to the direct server, gateway, and URL factory helpers. Cover public passthrough and old-native-library errors with fake-library tests.
Add a Python header example showing create_with_url with runtime access_token and headers options. Default the MCP URL to the local gateway address used by the token-flow examples and keep examples/api unchanged.
Add create_by_url_run.sh for the Python header example using the copied local gateway and MCP server helpers. The runner releases ports 5000/5001, starts the helpers with GOPHER_SDK_TEST token-flow logging, runs deterministic MCP curl calls, and optionally runs the Python SDK client when ANTHROPIC_API_KEY is available. Copy create_by_url_gateway and create_by_url_server helper executables from the gopher-mcp-js header example.
Summary: - search installed platform native packages before cwd/local development paths in both FFI loaders - keep local build loading behind explicit environment overrides such as GOPHER_MCP_PYTHON_LIBRARY_PATH - update search-order tests and runtime-options fake signature for the current API Verification: - git diff --check - python3 -m pytest tests/test_library_search_paths.py tests/ffi/auth/test_loader.py tests/test_ffi.py tests/test_ffi_runtime_options.py -q
Summary: - normalize empty access_token values to absent across mapping, object, and builder paths - avoid generating Authorization: Bearer with an empty token - omit empty access_token from the dynamic header create_by_url example - add config and FFI regression coverage for empty access tokens Verification: - git diff --check - python3 -m pytest tests/test_config.py tests/test_ffi_runtime_options.py tests/test_agent_runtime_options.py -q
Summary: - explain why encoded header bytes are pinned while populating c_char_p fields - document that native BuildAgentOptions deep-copies runtime options during agent creation - clarify that Python storage only needs to survive the FFI create call Verification: - git diff --check - python3 -m pytest tests/test_ffi_runtime_options.py -q
Summary: - add an explicit RuntimeOptionsInput type for public runtime option signatures - normalize direct factory runtime options before FFI calls to match builder behavior - reject invalid direct-factory runtime options before native invocation - update tests to assert normalized options instead of raw object passthrough Verification: - git diff --check - python3 -m pytest tests/test_agent_runtime_options.py tests/test_config.py tests/test_ffi_runtime_options.py tests/test_ffi.py -q
Summary:\n- move runtime option types and normalization into a shared low-level module\n- keep config as a consumer while preserving public exports\n- raise AgentError for direct FFI missing-symbol failures\n- update FFI tests for SDK error hierarchy
Summary:\n- restore parent-recorded submodule SHAs as the default build behavior\n- add GOPHER_ORCH_TRACK_REMOTE=1 for explicit branch-tip tracking\n- apply the same pinned/remote choice to nested gopher-mcp updates
Summary:\n- create private mktemp log directories and remove logs after successful runs\n- stop killing arbitrary listeners on default ports\n- auto-select local ports and retry on bind failures\n- verify Authorization reachability without depending on exact native token log formatting
Summary:\n- add regression coverage for create_by_url runtime options from env\n- assert empty GOPHER_ACCESS_TOKEN omits access_token and Authorization\n- assert non-empty tokens still use the access_token convenience path
Summary:\n- delete the committed Mach-O header example server and gateway binaries\n- build the required gopher-orch header fixture targets on demand from the runner\n- ignore the old local binary paths so generated artifacts are not re-added\n\nVerification:\n- bash -n examples/header/create_by_url_run.sh\n- python3 -m pytest tests/test_header_create_by_url_example.py
de69016 to
d43201e
Compare
No description provided.