Repository navigation
Run Codex discovery as a trusted command hook - #153
Merged
Merged
Conversation
|
Review: approved for merge pending the maintainer. Checked the helper adapter (bounded EOF input, session/turn keying, unmatched Codex callbacks do not refresh the deadline), exact legacy migration in place with Post appended, position-independent native verification and trust of exactly the two current keys, readiness for legacy-only and partial definitions, and the docs. The isolated Codex 0.160.0 contract and restart evidence matches ADR 0009. Real Orca acceptance remains in #127. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #152
Summary
askkey hook codexwith bounded EOF JSON, explicit session/turn isolation, and the shared SSH reminder; successful catalog callbacks settle the reminder, while failures or missing callbacks release after 30 seconds without progress.Diff stat
git diff --stat origin/main: 18 files changed, 815 insertions(+), 292 deletions(-).Head:
3c17325351c44afc160be405a5c1537dbd5c26b7(pushed and confirmed bygit ls-remote). Base:e24d953.Tests
Local commands, with no top-level
ASKKEY_*overrides:swift buildandswift test --filter 'CodexNativeHookClientTests|CodexOnboardingSetupTests'(26/26, repeated after extracting the fixture to meet the 600-line limit). The final no-progress refinement repeated build plus all three command-hook suites (23/23): unmatched Codex Post callbacks cannot refresh the deadline; other clients retain their behavior. All runs had zero failures and zero skips. These runs cover 86 distinct final tests, including unchanged Claude/Cursor/Grok behavior, approval decisions/allowances, and the retained legacy MCP guard.origin/maintest definitions: 20 added tests; existing counts retained. Main was not rebuilt/retested locally. The full suite and desktop flows are reserved for PR CI.Checks
python3 scripts/check_hygiene.py: passed (size=0,test-support=0,debug=0,local-path=0,non-ascii-name=0,multica=0,cjk=0).python3 scripts/check_module_deps.py: passed.git diff --check: passed.mkdir /tmp/askkey-build-slot-1before local builds/tests; no local full-suite run, app launch, screenshots, UI automation, orrun-e2e.sh.3c17325351c44afc160be405a5c1537dbd5c26b7: build-and-test green (8m49s); basic-ui-flows green (8m31s). All workflow steps passed, including required desktop flows, optional screenshots, release-symbol checks, optimized cancellation runtime, automation checks, development launcher, hygiene, and module dependencies.xcodebuild,swift-frontend,testmanagerd, andlaunchd_simprocesses remain active outside this task, so the task's.build(about 1 GiB) is retained under the disk-hygiene rule until those processes are idle or the coordinator removes this worktree after merge. No task build/test process remains; today's XCTest device directories are preserved.Codex 0.160.0 contract and isolated real process check
Coordinator approved the contract before parser/configuration edits. Primary source: Codex tag rust-v0.160.0, peeled commit
a956835d020762cb2b570053af06f643a11c0ecc.Source checks:
codex-rs/hooks/schema/generated/{pre,post}-tool-use.command.input.schema.json,hooks/src/events/pre_tool_use.rs,hooks/src/engine/command_runner.rs,core/src/tools/{hook_names,registry}.rs, andcore/src/tools/handlers/mcp.rs. They establishUserPromptSubmit,PreToolUse, andPostToolUse, explicitsession_id/turn_id/tool_use_id, shell identityBashwithtool_input.command, catalog identitymcp__askkey__list_credentials, and the exit-0 denial envelopehookSpecificOutput.{hookEventName:PreToolUse,permissionDecision:deny,permissionDecisionReason:...}. Empty stdout allows;timeout:3covers command stdin/output execution and times out as a non-blocking failure. There is noPostToolUseFailure; the tool registry emits Post callbacks only for successful tool results, so failed/missing callbacks use the 30-second no-progress release.The real-process check used a freshly created temporary home and a minimal environment, with no model turns, authentication, MCP startup, or installed helper execution. Commands (temporary paths represented by variables):
Created only temporary
config.toml([features] hooks = true) andhooks.jsoncontaining one group in each ofPreToolUseandPostToolUse, with matcher^(Bash|mcp__askkey__list_credentials)$and handler{type:command,command:"\"/Applications/Ask Key.app/Contents/Helpers/askkey\" hook codex",timeout:3}. The helper path was definition text only.JSONL RPC sequence:
initializewithexperimentalApi:true;initialized;hooks/listwith the temporary home ascwds;config/readwithincludeLayers:true;config/batchWriteagainst the temporary user file with its returnedexpectedVersion,reloadUserConfig:true, and anupsertofhooks.statefor exactly the two returned keys (enabled:true,trusted_hash:<currentHash>);hooks/listreadback. Then terminate and start a fresh app-server twice, repeating initialize/list without writing trust again.Observed output:
sha256:36a292facf0ac00779326135032dc4bcaa83fde4580b1b8822b8dafb18322a45sha256:bb0257288466d136e4d680816417367e69cfbf3cb36730c842059f3352ae5b21The empty-file index is only an observation: implementation selects handlers by exact event/matcher/type/command/timeout metadata and uses their returned keys/hashes. Tests put unrelated groups before and after ours and preserve their disabled state plus stale legacy state outside the two current keys.
A second isolated check started with the exact legacy MCP group, trusted it through the same app-server RPCs, terminated the process, replaced the group in place with both command definitions, and started a fresh app-server. It confirmed that the legacy and new Pre handler reuse the same positional key, while the new definition is reported as
modified(nottrusted). Explicitly trusting the new current hashes succeeded, and two further fresh starts both remained trusted with those same new hashes:The coordinator explicitly approved the key-reuse interpretation: do not reuse the old hash; update only the two current command keys, including a reused legacy key, during explicit Connect. Every other entry stays untouched. Regression coverage verifies
modifiedis reconnect/untrusted before Connect, writes the new hash to the reused key, and completes onboarding after fresh trust.Deviations and questions
None in implementation scope; the coordinator-approved clarification for reused native trust keys is recorded above. Real repeated Orca-session trust/discovery acceptance remains the maintainer's installed-release task in #127; source/unit/standalone app-server checks do not establish that outcome. No production vault, real credentials, real Codex home, Orca runtime home, or installed app was read or modified.