CRW-601: port the hook trust entry listing from CXC v0.2.40 - #567
Merged
Merged
Conversation
Add hooktrust_entries_test.go with the five hook-trust.test.ts B tests (:127-186), a replay of 200 answers recorded from the CXC v0.2.40 dist by record-entries.mjs (entries-oracle.json), and an exact-bytes test for a reference that holds a lone surrogate. ListHookTrustEntries is a stub that returns an error, so 194 of the 200 recorded cases and all seven top-level tests fail; the port follows in the next commit.
Replace the stub with ListHookTrustEntries (hook-trust.ts:48-57 and :140-216): read the plugin manifest, follow each declared hook file under the oracle's containment rules (lexical and realpath, a second "./" strip, absolute results of it), list events in Object.entries order, skip groups whose matcher is not a valid RegExp (regexp.Compile, or a failure caused only by lookaround or backreferences) and handlers that are not command hooks, then key and hash each one with HookTrustIdentityHash. All 200 recorded oracle answers and the seven top-level tests pass.
Append the port: kept lines for the matcher grammar residual, the inherited-function event spelling, non-object hooks values, the double "./" strip, the unchecked manifest path and the reader depth limit to docs/port-cxc/known-defects.md, and register the three corpus fixtures that reach listHookEntries as pending in contract/notes/cxc/CRW-601.json: none of them can run before the CLI entry points exist.
Use os.tmpdir() instead of a fixed /var/tmp fallback and drop the blank line at the end of record-entries.mjs. The recorded answers are unchanged (the recorder was re-run and its output compared byte for byte).
A probe of about 1,350 generated documents against the oracle found no difference beyond three more matcher grammar residuals (a group name with a non-ASCII letter, a lone surrogate in a matcher, a repeat count above 1000) and the reader depth limit, all recorded now. Mutating the port to return earlier entries together with an error survived the replay, so the replay asserts that an error never comes with entries, and a case where a later file's document fails after an earlier one listed entries joins the fixture. Also note that a FIFO blocks the read in the oracle and in the port.
|
You have reached your Codex usage limits for security reviews. Please try again later. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Devin's review reported two security findings and one test weakness. - The manifest was read without the containment check the hook files get, and a hook file was opened after its path was checked, so a symlink put in place between the two was followed. hookTrustEntriesReadContained now checks the manifest like any hook file and, once a file is open, resolves its path again and refuses a file that is not the one it opened or no longer lies inside the plugin root. The oracle's answer for a manifest that leaves the root stays in the fixture as intentionally_changed_* cases, and a swap test (a regular file and a symlink to an outside file exchanged while the listing runs) fails on the previous code in every run and passes on this one. known-defects.md records the line as port: fixed. - The replay accepted any error for a SyntaxError case, so a missing-file error passed; it now refuses a file-system error there. - Two more oracle quirks are recorded as port: kept: a FIFO blocks the read, and a plugin root of / rejects every reference.
The coordinator refreshed the base. The only conflict was in docs/port-cxc/known-defects.md, where both sides appended lines at the same place (1 block(s)); both sets are kept, dev's lines first, then this branch's.
This was referenced Oct 5, 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.
What this changes
Adds
ListHookTrustEntries(pluginRoot, pluginKey string) ([]HookTrustEntry, error)tointernal/runtime/doctor: it lists the command hooks a plugin declares, as{Key, Hash, FileSha256}per hook, so a later change can compare them with the trust records Codex keeps inconfig.toml. It is a port oflistHookEntriesfrom CXC v0.2.40 and writes nothing: a read-only function with no caller yet, so nothing is registered, activated or installed.Expected behaviour (each point is a recorded oracle case or a test; the oracle is CXC v0.2.40, commit 3c1459acadeb1906d97c00a598e1457327ae372d):
",\, CR and LF` ("... contains characters unsafe for a TOML quoted key")..codex-plugin/plugin.jsonhooksthat is not an array lists nothing; a non-string reference is refused../, must be relative, and must stay inside the plugin root both lexically and after symlinks ("must be relative", "escapes plugin root", "symlink escapes plugin root"); the file's SHA-256 isFileSha256.Buffer.toString("utf8")thenJSON.parse, throughinternal/pyjsonwith surrogates kept): events in the file's key order (array-index keys first, asObject.entrieslists them), then groups and handlers in array order, with the oracle's structural errors in the oracle's order.*matcher is not a valid JavaScript RegExp is skipped (see the defects below for how Go approximates that); a handler is skipped when its type is notcommand, its command is not a string or is blank after JavaScripttrim, orasyncis true; every other handler gets the key<pluginKey>:<path>:<event label>:<group>:<handler>andHookTrustIdentityHashof the hook (CRW-347: port the hook trust identity hash #560).Oracle sources ported
plugins/codexclaw/components/cxc-ops/src/hook-trust.tslines 48-57 (HookEntry) and 140-216 (normalizeHookPath,containedPluginFile,assertSafeHeaderValue,listHookEntries), and the fivehook-trust.test.tstests at 127-186. Recorded answers come from the CXC v0.2.40 dist throughtestdata/hooktrust/record-entries.mjs(Node v24 is test data only; the Go tests replayentries-oracle.jsonand never run Node).Tests and evidence
0b80f1443adds the tests against a stub that returns an error: 194 of the 200 cases recorded at that point and all seven top-level tests fail (the six that pass are JSON syntax errors, which the stub's error satisfies). The port follows in the next commit.go test -count=1 ./internal/runtime/doctorpasses: the five ported B tests, an exact-bytes test for a reference holding a lone surrogate, and a replay of 206 recorded oracle answers (122 entries in total) covering the real CXC plugin (31 hook files, 32 entries), a snapshot of this repository's plugin, every guard above, theObject.prototypeevent names, encodings (invalid UTF-8, BOM, lone surrogates), path containment (dotdot, absolute, symlink to file and directory, symlinked manifest, sibling-prefix, symlinked root, relative root), and matcher validity.TestListHookTrustEntries_swapAfterTheCheckIsNotFollowedexchanges the hook file between a regular file inside the root and a symlink to an outside file while the listing runs. The first version of the port listed the outside file in 5 of 5 runs of the test; the current one listed it in none of 240,000 listings../strip, decoding withstrings.ToValidUTF8, event order, surrogate path,TrimSpace, matcher rewrite character,*matcher, null versus absent matcher, key spelling, async skip dropped, lookahead rewrite, digest of decoded text, error ordering, partial entries on error) each make the replay fail.make lint(vet, staticcheck, gofmt),GOOS=darwin go vet ./...,CGO_ENABLED=0 go build ./...,go run -tags dev ./cmd/crw-dev ci validate,ci contractsandgo test -count=1 -run TestDomain/cxc ./internal/contracttestpass;ci plugin(no--record-version) reports the same package digest as the base (635105c68ea07f89), and no file underplugins/or any.codex-plugindirectory changed. Noinitfunction, package-level variable, cgo or Node at run time. Secret scan of the pushed range with the CI gitleaks version: no leaks.Corpus fixtures that reach this unit
listHookEntriesis called fromdiagnoseHookTrustandretrustHooks(hook-trust.ts:333, :421). Three fixtures ofcontract/fixtures/cxcreach it; none can pass with this PR alone, because their entry points are thehooks retrustanddoctorcommands and theconfig.tomltrust reader that follow. They stay pending and are listed as such incontract/notes/cxc/CRW-601.json(nothing is claimed identical or intentionally changed):cli__hooks__retrust_needs_bootstrap_then_writeshooks retrust(retrustHooks lists the hooks, then writes trust sections)cli__hooks__retrust_safety_pin_refuses_all_driftedhooks retrust(lists, compares with the recorded hash)cli__doctor__codex_bin_and_surface_envdoctor --jsonhook-trust check (32 hooks listed) and itshooks retruststepThe other fixtures that print a hook-trust check stop before listing ("enabled install key is ambiguous (0)"), and
cli__hooks__retrust_without_install_key_failsfails at key resolution; none of them runs this unit.Size
1,095 counted lines. The issue's own limit is about 1,035; the operator's allowance for finished and verified work reaches about 1,190, and the excess is tests plus the fix for Devin's two security findings. Composition: implementation
hooktrust_entries.go470; testshooktrust_entries_test.go322 and recorderrecord-entries.mjs283 (605); documentationknown-defects.md10 and the corpus notes file 10 (20). Not counted: the recorded fixtureentries-oracle.json(2627 lines, 124 KB, generated by the recorder; re-running it reproduces it byte for byte).Defects found in the oracle (recorded in
docs/port-cxc/known-defects.md,port: keptunless noted)regexp.Compileaccepts, plus what it refuses only for lookaround or backreferences); the lines list the measured residual differences in both directions (measured against Node 24 over 96 patterns, 14 pinned asmatcher_residual_*cases).Object.prototypedefines passes the oracle'singuard and is spelled into the key as the inherited function's source.hooksis read as an object with index keys, a number or booleanhooksis an empty list, a JSONnulldocument throws a raw TypeError, and integer-like keys come first../is stripped twice, so././x.jsonis keyed./x.jsonbut read fromx.json, and././/x.jsonis refused as escaping the root.port: fixedafter Devin's two security findings: the manifest was read without the containment check, and a checked hook file was opened after its path was checked, so a symlink swapped in between was followed. The port applies the same containment to the manifest and, once a file is open, resolves its path again and refuses a file that is not the one it opened or no longer lies inside the root. The oracle's answer for a manifest that leaves the root stays in the fixture asintentionally_changed_*cases./rejects every reference, because the prefix test appends the separator to a root that already ends with one.Not in this PR
The
config.tomltrust reader andretrustthat call this function, CLI wiring, and anything underplugins/(no hook or skill is registered). Engine error texts (file not found, directory, JSON syntax) are Go's, not V8's: the tests compare their class.Review notes
The implementation plan was audited by an independent reviewer over three rounds before any code was written (two FAILs folded in, then PASS). A separate model review of the final diff could not be completed: the provider's content filter stopped the request before a verdict, and the dispatch protocol offers no retry for that code. What that review had already produced was read, each difference it listed was re-checked against the oracle, and the cases and the one assertion it showed missing were added. Devin reviewed the first head (five comments: two bug, one analysis, two security). Both security findings and the test weakness are fixed in the second commit after the first push (bd4a69b); the two bug comments (the
/root quirk and the matcher grammar) are oracle behaviour recorded in known-defects.md; all five threads are answered and resolved. Codex's Code Review completed on the first head with no comments; its Security Review was skipped for a usage limit. The maintainer's review covers the rest.Built on
devat 270a056;docs/port-cxc/known-defects.mdis append-only, so the branch may show a conflict there with a newerdev(both sides add a section at the end).