Skip to content

🐛 fix(search): reject out-of-range --decay-floor and negative --half-life - #72

Merged
cwest merged 1 commit into
mainfrom
wt/t_57a11ffa
Aug 3, 2026
Merged

🐛 fix(search): reject out-of-range --decay-floor and negative --half-life#72
cwest merged 1 commit into
mainfrom
wt/t_57a11ffa

Conversation

@cwest

@cwest cwest commented Aug 3, 2026

Copy link
Copy Markdown
Owner

Closes #71.

What

--decay-floor landed in f4c9824 with no upper bound. The recency multiplier is

return math.Max(math.Pow(0.5, ageDays/d.HalfLifeDays), d.DecayFloor)

so any floor above 1 wins the math.Max for every node and becomes a flat GAIN on
raw cosine — a flag whose help text reads "lower clamp on the recency multiplier"
silently amplifies scores out of the [-1, 1] cosine range. A negative floor
silently re-enables the exact recency inversion f4c9824 fixed. A negative
--half-life was accepted silently too, byte-identical to no decay, while the HTTP
surface (internal/apiserver/search.go) already rejects half_life < 0 with 400.

This validates both at parse time in cmd/okfctl-search/main.go before
search.DecayOptions is built:

  1. --decay-floor outside [0, 1] (inclusive) is rejected, error names the flag and the range.
  2. Negative --half-life is rejected, reusing the apiserver's must be a non-negative number of days wording so the two surfaces agree.
  3. One error per invocation; --decay-floor is validated first, so a both-bad invocation yields a deterministic single error (pinned in a test).
  4. factor() doc comment (internal/search/query.go) now records that DecayFloor is required in [0, 1] and the CLI enforces it, so [DecayFloor, 1] is a real interval by construction.
  5. No library-side re-clamp — a silent clamp would change behavior for in-range values, the very "quietly rewriting it" the issue rejects.
  6. One-line freebie: the stale #65 not landed comment on the API search handler is corrected.

Tests — both directions proven

New table/tests in cmd/okfctl-search/main_test.go. Positive controls assert the
message text, not just the exit code; the three named existing controls pass
unmodified.

$ go test ./cmd/okfctl-search/ -run 'TestPlugin_DecayFloor|TestPlugin_HalfLife|TestPlugin_BothFlagsBad|TestPlugin_MinRelevance' -v
--- PASS: TestPlugin_HalfLifeAcceptedAndUnsetUnchanged        (unmodified)
--- PASS: TestPlugin_DecayFloorPositiveControl                (unmodified)
--- PASS: TestPlugin_DecayFloorNegativeControl                (unmodified)
--- PASS: TestPlugin_MinRelevanceBothDirections
--- PASS: TestPlugin_DecayFloorRejectedOutOfRange
    --- PASS: .../above-one-1.5
    --- PASS: .../above-one-2
    --- PASS: .../negative-1           (comment records the #65 regression vector)
--- PASS: TestPlugin_HalfLifeRejectedNegative
--- PASS: TestPlugin_BothFlagsBadDeterministicSingleError
--- PASS: TestPlugin_DecayFloorBoundariesAccepted  (0 and 1 both accepted)
ok  	github.com/cwest/okfctl/cmd/okfctl-search

Gates (all three layers)

$ go test ./internal/okf/ ./cmd/ -run Conformance -race -v
ok  	github.com/cwest/okfctl/internal/okf	1.228s
ok  	github.com/cwest/okfctl/cmd	1.409s

$ gofmt -l .          # (empty — clean)
$ go vet ./...        # (clean)
$ go test ./... -race
ok  	github.com/cwest/okfctl/cmd	9.416s
ok  	github.com/cwest/okfctl/cmd/okfctl-api	2.371s
ok  	github.com/cwest/okfctl/cmd/okfctl-search	1.855s
ok  	github.com/cwest/okfctl/internal/apiserver	1.701s
ok  	github.com/cwest/okfctl/internal/okf	(cached)
ok  	github.com/cwest/okfctl/internal/okfconfig	(cached)
ok  	github.com/cwest/okfctl/internal/plugin	(cached)
ok  	github.com/cwest/okfctl/internal/search	1.943s

Real-corpus controls — ~/src/knowledge-base/bundles/knowledge (262 .md files)

Base = main @ e63969b; after = this branch. Decay is a no-op on this corpus
(zero nodes carry generated.at), so the good-input controls must be identical
before/after — proving the validation does not fire on good input:

# Control Before After
11 validate OK: bundle conforms to the OKF spec floor OK: … (identical)
12 lint --strict line count 4 4
13 --semantic "docker container runtime" --k 20 --half-life 90 40 lines byte-identical (diff clean)

Top-5 (unchanged): 0.3749 infra/serverless-gpu-compute-modal-vs-gke.md,
0.3426 research/ai-fluency.md, 0.2981 research/agent-skill-app-substitution-evidence.md, …

The fix fires on the same corpus with bad input (before: silent exit 0):

$ okfctl-search --semantic "docker container runtime" --half-life 90 --decay-floor 3 .   # (base) 0.3749 … exit 0
$ okfctl-search --semantic "docker container runtime" --half-life 90 --decay-floor 3 .   # (after)
okfctl-search: --decay-floor must be in [0, 1], got 3
exit 1
$ okfctl-search --semantic "docker container runtime" --half-life -5 .                    # (after)
okfctl-search: --half-life must be a non-negative number of days, got -5
exit 1

…life

--decay-floor landed with no upper bound. The recency multiplier is
math.Max(0.5^(age/half-life), DecayFloor), so any floor above 1 wins the
max for every node and becomes a flat GAIN on raw cosine — a flag whose
help text reads "lower clamp on the recency multiplier" silently turns
into a score amplifier, and scores leave the [-1, 1] cosine range the
rest of the ranking assumes. A negative floor is worse than redundant: it
silently re-enables the exact recency inversion f4c9824 was merged to fix
(an old-but-relevant node crushed to 0.0000 below a mediocre fresh one).
A negative --half-life was accepted silently too, byte-identical to no
decay, while the HTTP surface already rejects half_life < 0 with 400.

Validate both at parse time in okfctl-search before DecayOptions is
built: reject --decay-floor outside [0, 1] (inclusive) and a negative
--half-life, reusing the apiserver's "non-negative number of days"
wording so the two surfaces agree. --decay-floor is validated first, so a
both-bad invocation yields one deterministic error. No library-side
re-clamp — a silent clamp would change behavior for in-range values, the
very "quietly rewriting it" this rejects. The factor() doc comment now
records that DecayFloor is required in [0, 1] and the CLI enforces it, so
[DecayFloor, 1] is a real interval by construction; the stale
"#65 not landed" note on the API search handler is corrected in passing.

Closes #71

@cwest cwest left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No changes needed.

The fix rejects --decay-floor outside [0, 1] and a negative --half-life at parse time, before DecayOptions is built, which is where the API surface already draws the line. The [0, 1] bound is inclusive on both ends: a floor of exactly 1 (no decay) is accepted, and 0 restores unbounded decay. When both flags are bad only the --decay-floor error surfaces, and that ordering is pinned by a test so a reorder can't silently change which message a user sees.

I ran the whole thing end to end against a freshly built binary, not just the unit suite. Bad input fires: --decay-floor 2 and -1 exit 1 naming the flag and the range, --half-life -5 exits 1 with the same non-negative wording the HTTP handler uses, and both-bad yields the single deterministic decay-floor error. Good input stays silent: the two boundaries, the 0.25 default, and the flags-omitted path all exit 0. The three existing controls pass unmodified, and the negative case carries a comment recording that a negative floor re-opens the #65 inversion, so the test is guarding a real regression vector rather than restating the zero case.

gofmt is clean, go vet is clean, the full -race suite passes across all packages, and the conformance gate is green. The apiserver change is the one stale-comment correction and touches no logic. Scope holds — no HTTP plumbing, no default change, no --min-relevance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🐛 fix(search): reject a --decay-floor above 1 instead of amplifying scores

1 participant