Skip to content

sec eval s1 --effort is inert: the override is set but nothing reads it #277

Description

@sroussey

The finding

sec eval s1 --effort <none|low|medium|high|extra|ultra> accepts a value, validates it against isModelEffort, and stores it — and then nothing in any production path ever reads it. The flag costs the operator nothing and changes nothing.

The chain, as it stands on main:

  • src/cli/groups/eval.ts parses --effort and passes it into EvalS1Task.
  • EvalS1Task.execute (src/task/eval/EvalS1Task.ts:159-171) validates it and calls setExtractionEffortOverride(...), correctly clearing it in a finally.
  • setExtractionEffortOverride stores it in a module-level effortOverride in src/sec/forms/registration-statements/s1/extractionReasoning.ts.
  • That variable is read only by withExtractionReasoning(model, effort) in the same module.
  • withExtractionReasoning is never called from any production path. git grep finds exactly two files referencing it: its own definition, and its own unit test.

So the module is fully unit-tested and fully unreachable. An operator running sec eval s1 --effort ultra gets a sweep identical to one run without the flag, with nothing saying so.

This traces to 2496d02 ("define the four symbols main already imports"), a stop-the-bleeding lift that landed setExtractionEffortOverride (along with personNameFields and disabled: true) without the consumers that give them effect. The sibling gaps are addressed in #275 and #276; this one is deliberately left open because the fix is not obviously correct.

Reference implementation

A working version exists on origin/trim-better-coverage (88d1ea0), in src/sec/forms/registration-statements/s1/sectionExtractors.ts:

  • :761const effectiveModel = withExtractionReasoning(model, reasoningEffort); inside runStructured, replacing the bare model from there on (it also feeds resolveModelId and the LOCAL_LLAMACPP grammar check);
  • :75, :776 — a REASONING_TOKEN_PADDING = 8192 added to maxTokens whenever reasoningEffort is set and not "none". That interaction is the non-obvious part and worth keeping: reasoning tokens are drawn from the same answer budget, so enabling reasoning at the shared MAX_TOKENS = 4096 would truncate the JSON mid-object — which for risk-factors means a failed chunk and a failed section.

Two things that are unproven

1. Do workglow's providers read ModelConfig.effort at all? withExtractionReasoning clones the config with a top-level effort. Nothing here establishes that any installed provider maps that onto its native reasoning knob. If they do not, wiring it up produces the same inert flag with more code behind it.

2. Can it work on OpenAI at all, given that extraction pins temperature: 0? Per the repo's own documentation of the coupling: gpt-5.6-luna rejects temperature alone with 400 Unsupported parameter but accepts {reasoning: {effort: "none"}, temperature: 0}, so the provider (finalizeResponsesRequest) turns reasoning off for any request that pins a temperature. Extraction pins temperature: 0 on every call by design (SEC_EXTRACTION_TEMPERATURE), because unpinned sampling made one filing yield 138/138/109 risk factors across three runs. So on OpenAI's reasoning families, --effort high and temperature: 0 appear to be mutually exclusive, and the provider silently resolves that conflict against the flag. SEC_OPENAI_REASONING_EFFORT exists to override the inference — whether it composes with this flag, or fights it, is exactly what needs measuring.

Why this is not just merged

runStructured is not eval-only — it serves every filing the production pipeline extracts. A change there that alters maxTokens or the model config reaches every S-1, 424, merger proxy, redemption and LOI extraction, not just a sweep somebody chose to run. That is a materially larger blast radius than the flag's benefit implies, and it should not ride in on an eval-ergonomics fix.

Recommendation

Pick one; do not leave it as it is.

  1. Port it behind a provider-capture test. Wire withExtractionReasoning into runStructured as on the reference branch, but first add a test that captures the request payload actually handed to the provider and asserts the effort survives into it — per provider family, and specifically alongside temperature: 0 for the OpenAI path. That test answers both unknowns above and prevents the flag from going inert again the next time a lift lands without its consumers.
  2. Remove the flag and the module. Delete --effort, setExtractionEffortOverride, withExtractionReasoning, and extractionReasoning.ts with its test.

An operator paying for --effort max deserves either a working knob or no knob. What they have now is a knob that reads as working — validated input, a clean finally, a fully green unit test — and does nothing, which is the worst of the three states.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions