Uh oh!
There was an error while loading. Please reload this page.
Add token passthrough feature - #12
Conversation
7045a5f to
2e1e79aCompare
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).
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
bb91598 to
a46cfc5Compare
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.
Uh oh!
There was an error while loading. Please reload this page.
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
d43201eCompare
No description provided.