Repository navigation
feat: MCP acceptance adapter (request_acceptance via client elicitation) - #445
Merged
Merged
Conversation
…gh elicitation Adds core/engineering/acceptance.ts and a `request_acceptance` action on codecarto_change. HostCapabilities are derived from the session (client capabilities + clientInfo at initialize) and host config, never from request fields; acceptanceChannelSupported decides whether the client may be asked (registry still empty, so every host returns needs-human-acceptance today); the decision comes only from the client's elicitation response via elicitationDecision; an approval is minted only for `accepted`, checked with evaluateApprovalReceipt against the stored request before persisting, and returned with classifyAcceptance's reading (cooperative on this host, D3). The SUPPORTED path is tested through an @internal buildServer option that only tests/helpers/acceptance-server.mjs passes; bin.mjs never does. Refs #409
…most one approval per candidate requestAcceptance never ran the acceptance gate and never re-read the tree, so it asked a person about — and minted approvals for — candidates the gate refuses (no review at all) and bytes that had moved since capture. It also minted a second accepted approval for one candidate, sequentially or concurrently. Review of #445 (Refs #409). - core/engineering/acceptance.ts: before any request is built, the caller's `reread` of the working tree is passed to evaluateAcceptanceGate as candidate_reread; unless the gate says may-accept, no request is issued and nobody is asked (`blocked` with the blockers via describeGateOutcome; `needs-human-acceptance` when the tree cannot be re-read). An accepted approval already bound to this candidate's id+digest refuses with invalid-state/already-accepted. Request→present→mint is serialized per attempt by an O_EXCL marker `requests/.in-flight` carrying the request's expires_at; a marker past it is cleared. The store's change lock is not held across the elicitation wait. - mcp-server/working-tree.ts (new): re-reads cwd in the candidate's scope (same exclusions, `.git/` skipped, HEAD read from .git/HEAD; no git process). Both `gate` and `request_acceptance` supply it. - mcp-server/server.ts: buildServer() is the only public overload; the options overload is @internal, so the published .d.ts no longer names a stripped interface (TS2304 under skipLibCheck: false). - tests: gate-refused, edited-tree, gate-action-on-edited-tree, sequential and concurrent duplicate, stale marker, forged elicitation content (M6), and a consumer compile of dist with the repo's tsc and skipLibCheck off. acceptance moves to layer 5 in the layer map (it now imports gates.ts). - docs: registry entry shape after a live check; the assurance wording wart.
The new "published declarations compile" test failed on test-windows with TS2307. It imported dist through pathToFileURL(...).pathname, which on Windows is "/D:/a/...", a path tsc cannot resolve. The consumer now lives in a scratch directory under node_modules/.cache and imports dist by a relative specifier. A relative path from the OS temp dir does not work either, because the runner's temp dir is on C: and the checkout is on D:. The scratch directory is removed afterwards. Checked that it still catches the bug it exists for: putting back the dangling BuildServerOptions reference in dist fails the test with TS2304.
…dapter Both PRs added a codecarto_change action and a new core/engineering module, so four files conflicted. Each resolution keeps both sides: - core/engineering/index.ts re-exports acceptance.ts and host-observations.ts. - mcp-server/engineering.ts: CHANGE_ACTIONS has request_acceptance and ingest_observations. The dispatch keeps gate's working-tree re-read (cwd) and adds both cases. The attempt_id description names all three actions. The requestAcceptanceAction body kept its closing brace, which the conflict hunk had dropped. - tests/engineering-contract.test.mjs: acceptance and host-observations both sit at layer 5, both are listed as impure, and both are allowed the fs/path/crypto imports. Build clean, npm test 1347/1347 twice, packed smoke 12/12.
…mtime, read HEAD through a worktree's gitdir The second review of #445 found three medium findings and one gap the pilot would hit on day one; each is reproduced by its probe before the fix and pinned by a test that kills the corresponding mutant. Re-read AGAIN before minting (review #4). The gate and the tree re-read ran once, before the request; nothing re-checked after the person answered, so a tree edited while the form was open still minted an approval bound to a candidate the tree no longer matched. requestAcceptance now runs args.reread again after elicitationDecision says accepted and checks it with checkCandidateFreshness; a mismatch or a read failure is `refused` with a reason starting `proof-stale:`, the stored request is kept, nothing is minted, and the marker is released. This is the contract's "before issuing AND AGAIN BEFORE MINTING". Unparseable in-flight marker (review #5). JSON.parse on an empty or truncated marker threw into the outer catch, which set stale=false, so the mtime fallback the comment promised was unreachable and the attempt was wedged forever. The parse is now its own try; an unparseable marker (or one without a finite expires_at) is judged by lstat().mtimeMs against the contract's maximum request TTL. Only a regular file is ever cleared: a directory or a symlink at the marker's path is refused by name with invalid-state and never removed (no rm -r). Worktree layout (review, recommended). A `.git` FILE (`gitdir: <path>`) failed closed with "no HEAD could be read". readWorkingTree now resolves the gitdir line (absolute or relative to the worktree root), reads HEAD from that git dir, and resolves a symbolic ref through the per-worktree dir first and then the `commondir` (relative to the gitdir) for shared loose refs and packed-refs. Anything malformed still refuses with the same reason; nothing outside what gitdir:/commondir name is followed; no git process is spawned (the module imports no child_process). Guards for the five mutants that slipped past (review #8): core-level calls with reread undefined and with a failing reread both stop at needs-human-acceptance with nothing presented; real-file tests pin the executable bit, symlinks recorded as links and never followed, the candidate's own exclusions, and unreadable files reported as uncovered. Refs #409.
…445) test (t) failed only on test-windows. After switching the worktree back to `feature` it expected FRESH and got STALE. The fixture passed GIT_CONFIG_GLOBAL=/dev/null and GIT_CONFIG_NOSYSTEM to the main-checkout calls only. The two checkouts inside the worktree ran under the runner's own config. On Windows that includes system core.autocrlf=true, so a.txt was rewritten with CRLF. The byte-exact re-read rightly called the tree STALE. /dev/null is not a path on Windows either. Reproduced on Linux with GIT_CONFIG_SYSTEM pointing at a file setting core.autocrlf=true: the original test fails at the same line (902) and the fixed test passes. The file now imports tests/helpers/git-environment-isolation.mjs first, as every other test that builds a git fixture does. It is added to that helper's guarded list (seven files, now eight), so the "imported first" check covers it. Every git call in the fixture uses one environment. This change is to the test only. readWorkingTree and its CRLF handling are unchanged, and byte-exactness is intended. npm test 1357/1357 twice.
The third review left four guard branches in mcp-server/working-tree.ts that no test covered. A mutant that removed any one of them still passed 33/33: - gitdir must be a real directory - the sha must be exactly 40 lowercase hex - '..', leading '/' and backslashes are refused in a ref - empty segments are refused in a ref Test (u) builds each malformed HEAD, ref or gitdir. Each refused case also plants a file the reader could resolve to a valid sha if its guard were missing, and each has a well-formed twin that is accepted. All four mutants now fail the test, as does a mutant that drops only the backslash check. The test is POSIX only (symlinks and backslash file names). npm test 1358/1358 twice.
On test-windows (runs for 14a8887 and 9438a73), the malformed-.git loop in (t) fails with EPERM when it opens <tmp>/wt/.git. Git for Windows marks a worktree's .git file hidden (core.hideDotFiles defaults to dotGitOnly), and overwriting a hidden file in place is EPERM on Windows. The loop now removes the file before writing it. The autocrlf fix from 14a8887 held: the failure moved past the FRESH assertion at line 902. Only the test changes; readWorkingTree is untouched.
This was referenced Sep 28, 2026
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
1 participant
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.
Refs #409
What
core/engineering/acceptance.ts:requestAcceptance— the contract's receipt path. Loads change/slice/attempt/candidate/proofs/reviews, builds + persists the request (requests/), callsacceptanceChannelSupported; unsupported →needs-human-acceptance, nothing presented. Supported → oneelicitation/createform (requireddecisionenum, optional note; TTL = request timeout, strictly below the registeredclient_request_timeout_ms), decision viaelicitationDecision(content.decision only), approval minted only foraccepted, checked withevaluateApprovalReceiptagainst the stored request before persist, returned withclassifyAcceptance's reading.codecarto_changegainsrequest_acceptance {cwd, change_id, attempt_id}.approve/acceptstay refused. Host-capability/decision fields on the request are refused by name.HostCapabilitiesderived inserver.tsfromgetClientCapabilities().elicitation.form+getClientVersion();assurance_policyalwaysverified;storage_boundary/tool_result_path/current_storagedefault to least-trusted (none) — no host-config mechanism exists yet.VERIFIED_ACCEPTANCE_INTEGRATIONSstays empty. Test-only seam:buildServer({ acceptanceRegistry })(@internal, stripped from d.ts), reachable only fromtests/helpers/acceptance-server.mjs;bin.mjs→startStdioServer()passes nothing; no env/config path.docs/engineering/acceptance-adapter.md.Tests (
tests/engineering-acceptance-adapter.test.mjs, real server over stdio, scripted client, real temp store) — 16 pass(a) empty registry → needs-human-acceptance, request on disk, no approval, 0 elicitations · (b) no elicitation capability → same · (c) registered 2.1.277 / live 2.1.283 → reason names both, 0 elicitations · (d) accept/accept → approval bound to stored request (nonce, digest, client, channel, host-session),
evaluateApprovalReceiptok, classcooperative, form schemarequired:["decision"]· (e) accept/reject → rejected, no approval · (f) missing / in-note / unknown decision → invalid · (g) decline, cancel · (h) never answers → timed-out (-32001) · (h') short client timeout bounds TTL; unusable timeout → nobody asked · (i) answer afterexpires_at→refusedreceipt-expired, no approval · (j) nonce tampered →refusedreceipt-mismatch · (k) approve/accept still refused with reasons · (l) 14 capability/decision fields refused, nothing stored.Mutation (each restored by content)
actioninstead of content.decisioncheckAcceptanceRequestTtlalways okGates
build 0 errors ·
npm test1311/1311 ×2 · tarball smoke 12/12 ·git diff --checkclean · leak grep emptyLimits
MRTR (2026-07-28
InputRequiredResult) not implemented —Presenterseam is where it plugs in · storage_boundary/tool_result_path/current_storage defaulted tonone· attempt record not rewritten toneeds-human-acceptance(attempts/ is create-only in the store; outcome carried by stored request + result) · no live client exercised · (i) stale is exercised at the storage seam because the transport excludes a late answer by construction (TTL = request timeout → (h)).