JAR-430: cover the hook scripts and their process contracts - #54
Merged
Conversation
Second of two, purely additive: no Makefile or CI change, since PR 1 wired the gate. Adds 113 Node tests over the token accounting and setting resolvers in lib/transcript-context.js, the sweep and session-id sanitising in lib/session-state.js, the cooldown split in check-console-log.js, and the regex-flag and glob handling in lib/utils.js. Process contracts for all four shipped hook entry points: exit 0 and either no stdout or exactly one well-formed JSON object, whatever arrives on stdin. sync-codex-agents.js is spawned rather than imported because hooks.json invokes it via require and it works at require time, so a require.main guard would make the hook a no-op; both plugins' copies are covered. The seam widens by three functions the suite consumes -- readFileTail and extractUsageTokens in transcript-context, filterByPatterns in utils -- which is the praxis version bump. Co-authored-by: multica-agent <github@multica.ai>
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.
Second of two for JAR-430. Purely additive: no Makefile or CI change, since #52 wired the gate. Stacked on #52 -- base is
agent/ferro/c4a8e65f, and GitHub retargets this tomainwhen that merges. Review #52 first.Unit tests
113 Node tests over the four modules
CONTRIBUTING.mdnames as class 1 -- pure logic behind a silent-wrong failure mode:lib/transcript-context.js(24) --extractUsageTokens, including the iterations-versus-top-level double count that reports a context 2.00x its real size;readFileTail;readLatestContextTokens, including the truncated-tail guard;resolveContextThreshold(0 disables, out-of-range falls back) andresolveContextInterval(0 does not disable, which is the contrast worth pinning).lib/session-state.js(17) --sessionIdsanitising and its fallback chain,sweepStaleState(TTL sides,keeppreserved, prefix isolation, never throws),stateTtlDays.check-console-log.js(10) --applyCooldown: fresh versus repeat, the cooldown reset, a finding that disappears dropping out ofseen, and a reintroduced statement counting as new again.lib/utils.js(24) --filterByPatternsincluding the all-invalid case that must not filter everything away; the regex-flag handling incountInFileandgrepFile, bothlastIndextraps;findFiles's glob-to-regex escaping.The sweep deletes files, so its tests redirect
TMPDIRat a throwaway directory first.os.tmpdir()reads the variable on each call rather than caching it, so that is enough, and it is the only way to keep a test that deletes files away from the real temp directory without mocking the module out from under the code being tested.Process contracts
One per shipped hook entry point, each spawned with malformed JSON, empty stdin and a payload with no
transcript_path, asserting exit 0 and either no stdout or exactly one parseable JSON object (JSON.parseis the "exactly one" half -- two concatenated objects do not parse). Each also has one positive case, so the "exactly one object" branch is actually exercised rather than only the silent one.sync-codex-agents.jsis spawned rather than imported for the reasonCONTRIBUTING.mdgives:hooks.jsoninvokes it viarequireand it works at require time, so arequire.mainguard would turn the hook into a no-op.PLUGIN_ROOTandCODEX_HOMEpoint at temporary directories throughout. Both plugins' copies are covered from the one file, discovered rather than hardcoded so a third copy is picked up -- a second file would only duplicate it, since the copies must be byte-identical andmake verifyalready asserts that.Seam
Three functions the suite consumes are added to their modules' exports:
readFileTailandextractUsageTokensintranscript-context.js,filterByPatternsinutils.js. That is the praxis1.4.1 -> 1.4.2bump and the regenerated.codex-plugin/plugin.json.Verification
Every test was checked to be load-bearing by reverting the behaviour it pins, one at a time, and confirming it reddens. 18 such mutations were run; each reddened the intended test and only it.
Three did not, and the reasons are worth recording rather than papering over:
PLUGIN_ROOTending in..case was built withpath.join, which normalizes the..away -- so it passed against the very bug it exists to catch. Rebuilt by concatenation, and it now catches it.sweepStaleState's strict>=is not observable. The cutoff is the clock at call time, so no mtime can be set to land exactly on it. The test pins the two sides one minute either way, and its comment says so rather than claiming the boundary itself.applyCooldown,state.seen.filter(key => present.has(key))cannot change any answer:seenis only ever queried with keys drawn fromfindings, which are exactlypresent, and the persistedseencomes from[...present]regardless. InsweepStaleState, droppingif (!entry.isFile()) continue;changes nothing either, sincermSyncwithoutrecursivethrows on a directory and the throw is already caught. Both are harmless and defensive. Neither is touched here -- editing behaviour-adjacent code in an additive test PR is the wrong place for that call, and they are reported rather than silently tidied.make test(139 Node + 40 Python, all pass) andmake verifypass locally.make lintpasses every hook that can run here; lychee, actionlint and zizmor need Docker, which is not reachable from this runtime, so thelintjob covers them.Refs #34
Closes JAR-430 -- this is the second and last PR the issue carries.
🤖 Generated with Claude Code