D8: declare indicators projected-but-unread and widen it to match what is produced - #28
Merged
Merged
Conversation
…ch reality Two type-only corrections to FroggyEnrichedView.technical.indicators, both surfaced by afi-reactor's D8 pass (the reactor↔afi-core boundary is now genuinely typechecked instead of ambient `any`). 1. Documentation gap. indicators' siblings atr14 and trendBias carry explicit "context only — NOT read by this adapter or any scorer input" annotations, and atrRegime documents that it IS read (AR-GOV D-AR-3). indicators carried no annotation at all, so a reader could not tell which it was. It is read by nothing: the only mentions of the token anywhere in afi-core are this declaration and an unrelated `indicators` array on the universal signal schema. Now annotated to match its siblings. 2. The declared type did not describe what is produced. The reactor's projecting node writes volume_ratio from TechnicalLensV1.volumeRatio, which is optional, so the key is written with an `undefined` value. The Record value type was `number | null`, which a Record position cannot widen away — the only field of the four to error. The distinction is load-bearing and is why the fix lands here rather than in the reactor: canonical JSON DROPS an undefined-valued key but RETAINS a null-valued one. Coercing with `?? null` in the reactor would have moved hashes; building the key conditionally would have changed Object.keys(). The only zero-behaviour-change option is to make the declared type honest. Type-only, proven: all 22 emitted dist/*.js files are byte-identical before and after. 231/231 tests green. Pairs with afi-reactor's D8 PR and must merge with it — the reactor now binds to this package's published dist/*.d.ts rather than to an `any` shim. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review correction. The comment justified widening with "a coercion that would
move hashes", which overstates the present situation: technical.indicators
reaches no live hash preimage today — enrichmentHash is computed over the raw
lane payloads and sorted categories, not over this renamed {rsi, ema_20,
ema_50, volume_ratio} projection.
The accurate justification, now recorded: widening is the only option that
provably changes nothing (coercing with `?? null` alters the value, omitting
the key alters Object.keys()), AND strategyLocalViewHash is a
declared-but-unproduced pin over exactly this view — so a coercion here would
silently become hash-relevant the moment that producer is wired up.
Still type-only: 19/19 emitted .js byte-identical (a clean build emits 19; the
previously-reported 22 counted three stale artifacts from sources deleted in
Mission R — dist/runtime/*.js and dist/src/dag/SignalEnvelope.js — which
`npm run build:clean` has now dropped). This matters beyond tidiness: afi-reactor
now typechecks against this dist/, so stale declarations here could mask or
invent errors across the D8 seam.
231/231 tests green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Kilo Code Review could not run — your account is out of credits. Add credits or switch to a free model to enable reviews on this change. |
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.
Type-only correction to
FroggyEnrichedView.technical.indicators, surfaced by afi-reactor's D8 pass (the reactor↔afi-core boundary is now genuinely typechecked instead of ambientany).What and why
1. Documentation gap.
indicators' siblingsatr14andtrendBiascarry explicit "context only — NOT read by this adapter or any scorer input" annotations, andatrRegimedocuments that it is read (AR-GOV D-AR-3).indicatorscarried no annotation at all, so a reader could not tell which it was.It is read by nothing. The only mentions of the token anywhere in afi-core are this declaration and an unrelated
indicatorsarray on the universal signal schema. The adapter reads onlyemaDistancePct/isInValueSweetSpot/brokeEmaWithBody/atrRegime. This survived five independent refutation angles during triage.2. The declared type did not describe what is produced. The reactor's projecting node writes
volume_ratiofromTechnicalLensV1.volumeRatio, which is optional — so the key is written with anundefinedvalue. TheRecordvalue type wasnumber | null, and aRecordvalue position has no optionality escape hatch, which is whyvolume_ratiowas the only one of the four indicators to error.Why the fix lands here rather than in the reactor
This was the deciding constraint. Canonical JSON drops an
undefined-valued key but retains anull-valued one:?? nullin the reactor → changes the produced value. Would move hashes.Object.keys().Only the third is zero-behaviour-change. (Precise scope: this view reaches no live hash preimage today —
enrichmentHashis taken over the raw lane payloads, not this renamed projection — butstrategyLocalViewHashis a declared-but-unproduced pin over exactly this view, so a coercion would become hash-relevant the moment that producer is wired up.)Verification
dist/*.jsbyte-identical before and after — proven type-only.dist/also cleaned: it held 3 artifacts (runtime/afiRuntimeAdapter.js,runtime/types.js,src/dag/SignalEnvelope.js) whose sources were deleted in Mission R, so it was not reproducible from source. That matters now that afi-reactor typechecks against thisdist/— stale declarations there could mask or invent errors.Governance
No filing required. Type-only, nothing in afi-core reads
indicators, and no scored value, hash, or golden byte moves. Recorded explicitly because it widens a published protocol-primitive contract consumed across repos.Full report:
reports/afi-d8-type-boundary-v0.1.md