Fix review findings: validate the hooks cap, integer read caps, doc drift, config plumb-through test
A Codex review pass on the draft caught four real gaps and two solid suggestions; all addressed except one pushed back on the merits: - hooks-claude/hooks-codex: stderrSummaryMaxChars was the one new knob with NO range validation — a negative/NaN cap would silently misbehave inside slice(). Both bridges now assert a positive integer at the TOP of apply() (before the config-file parse's early return, so a bad value fails the load loudly), with rejection tests. - tool-fs: the read caps count lines/chars/bytes, so positive-FINITE was too loose (a fractional readLimit would flow into windowing arithmetic and the schema description). All four now require a positive integer, matching tool-web's cap. - Doc drift the gates cannot catch: tool-web's README tools table still named WEB_SEARCH_MAX_RESULTS as the mechanism; compact-basic's README/module doc and the compaction-capability-seam RFC still described estimation as fixed char/4 rather than the charsPerToken default. - subagent-acp: the dispose graces were tested only at the startAcpRun level, so a regression that stopped threading plugin config into AcpRunSpec would have survived. A provider-path test now drives the trap-escalation scenario through ctx.subagents.start with small config graces and bounds dispose at 4s. Pushed back on: converting compact-basic's charsPerToken to a schemastery field. The package's whole config is deliberately hand-rolled (resolveConfig, every threshold REQUIRED with no default — a documented design posture); one schemastery field beside it would be incoherent. The knob is cordis.yml-reachable, defaulted, and validated, which is what the convention requires; migrating the package to schemastery wholesale is pre-existing config-surface hygiene out of this change's scope.
This commit is contained in:
@@ -68,7 +68,18 @@ function nextHandlerId(point: string): string {
|
||||
|
||||
const PLUGIN_SOURCE: MessageSource = { kind: 'plugin', plugin: 'hooks-codex' }
|
||||
|
||||
/** The summary cap bounds a persisted event field — a positive integer or the slice misbehaves silently. */
|
||||
function assertPositiveInteger(name: string, value: number): void {
|
||||
if (!Number.isInteger(value) || value < 1) {
|
||||
throw new Error(`hooks-codex: ${name} must be a positive integer`)
|
||||
}
|
||||
}
|
||||
|
||||
export function apply(ctx: Context, config: Config): void {
|
||||
// Validate the cap BEFORE the config-file parse: a bad value must fail the
|
||||
// load loudly, not be skipped by the parse-failure early return.
|
||||
const stderrSummaryMaxChars = config.stderrSummaryMaxChars ?? 500
|
||||
assertPositiveInteger('stderrSummaryMaxChars', stderrSummaryMaxChars)
|
||||
let parsed: CodexHookConfig = {}
|
||||
try {
|
||||
const raw: unknown = JSON.parse(readFileSync(config.configPath, 'utf8'))
|
||||
@@ -83,7 +94,6 @@ export function apply(ctx: Context, config: Config): void {
|
||||
}
|
||||
|
||||
const defaultTimeoutMs = config.defaultTimeoutMs ?? 600_000
|
||||
const stderrSummaryMaxChars = config.stderrSummaryMaxChars ?? 500
|
||||
const model = config.model ?? ''
|
||||
|
||||
async function runPoint(
|
||||
|
||||
@@ -207,6 +207,16 @@ describe('hooks-codex coverage — decision mapping paths', () => {
|
||||
expect(res?.type === 'hook/result' && res.data.stderrSummary?.length).toBe(501) // default 500-char cap + ellipsis
|
||||
})
|
||||
|
||||
it('rejects a non-positive or fractional stderrSummaryMaxChars at load', async () => {
|
||||
const d = dir()
|
||||
hooks(d, {})
|
||||
for (const bad of [0, -5, 1.5, Number.NaN]) {
|
||||
const adapter = new MockAdapter([])
|
||||
await expect(harness(join(d, 'hooks.json'), adapter, { stderrSummaryMaxChars: bad }))
|
||||
.rejects.toThrow(/hooks-codex: stderrSummaryMaxChars must be a positive integer/)
|
||||
}
|
||||
})
|
||||
|
||||
it('the stderr summary cap is plugin config (stderrSummaryMaxChars)', async () => {
|
||||
const d = dir()
|
||||
hooks(d, { PreToolUse: [{ hooks: [{ type: 'command', command: sh(d, 'l.sh', '#!/usr/bin/env bash\nprintf "x%.0s" {1..600} >&2\nexit 2\n') }] }] })
|
||||
|
||||
Reference in New Issue
Block a user