fix(stack): address EQL v3 types-module review follow-ups#559
Conversation
Resolves pending review feedback from #541 (merged). Seven findings: - test: restore the live ORE proof for text order domains. text_ord / text_ord_ore carry both hm (unique) and ob (ore), so the single-kind classifier ran only the eq_term/hmac_256 proof and silently skipped ord_term/ore_block_256 -- a wrong-valued ob would pass the matrix green. Run both proofs; build the ob term with queryType:'orderAndRange' since equality resolves to hm on these domains. - schema: extract a shared resolveMatchOpts() merge+clone helper used by both the v2 and v3 freeTextSearch builders, giving v2 the clone-on-write protection it lacked (it stored caller opts by reference). - eql/v3: derive EncryptedTextSearchColumn.build()'s unique/ore blocks via indexesForCapabilities (override only match) so the emitted index shape cannot drift from the shared capability mapping. - encryption/v3: precompute row reconstructors per schema table at typedClient construction and return a DecryptionError failure for unknown tables, so table.build()'s duplicate-column throw can no longer escape the decrypt Result contract as a promise rejection. - eql/v3: alias V3DecryptedModel to V3ModelInput (character-identical) to stop the input/output model shapes silently drifting. - ci: add src/schema/match-defaults.ts to the fta-v3 path scope -- it shapes every emitted v3 match block but sat outside the gate. - ci: re-baseline FTA --score-cap 72 -> 69 after the eql/v3 file split and refresh the spec doc's stale 71.08 baseline claim.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/stack/src/encryption/v3.ts (1)
187-193: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: freeze the shared failure object.
unknownTableFailureis a single shared object instance returned by reference on every unknown-table call. Since consumers won't mutate it in practice, this is low risk, butObject.freezewould guard against accidental mutation leaking across calls.🛡️ Optional defensive tweak
- const unknownTableFailure: { failure: EncryptionError } = { + const unknownTableFailure: { failure: EncryptionError } = Object.freeze({ failure: { type: EncryptionErrorTypes.DecryptionError, message: '[eql/v3]: decryptModel received a table this client was not initialized with — pass the same table object(s) given to EncryptionV3/typedClient', }, - } + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/stack/src/encryption/v3.ts` around lines 187 - 193, The shared unknown-table failure object in EncryptionV3 should be defensively immutable so accidental mutation cannot leak across decryptModel calls. Update the unknownTableFailure constant in v3.ts to freeze the top-level object (and, if needed, its nested failure payload) while keeping the existing EncryptionErrorTypes.DecryptionError message and structure unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/fta-v3.yml:
- Around line 21-26: Add the workflow file itself to the pull_request path
filters in fta-v3 so changes to .github/workflows/fta-v3.yml also trigger the
gate; update the pull_request paths alongside the existing
packages/stack/src/eql/v3 and schema/match-defaults entries so the workflow
validates its own changes before merge.
---
Nitpick comments:
In `@packages/stack/src/encryption/v3.ts`:
- Around line 187-193: The shared unknown-table failure object in EncryptionV3
should be defensively immutable so accidental mutation cannot leak across
decryptModel calls. Update the unknownTableFailure constant in v3.ts to freeze
the top-level object (and, if needed, its nested failure payload) while keeping
the existing EncryptionErrorTypes.DecryptionError message and structure
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 209ef5e6-2ad6-47c6-b6b9-f6e2e0f95e08
📒 Files selected for processing (9)
.github/workflows/fta-v3.ymldocs/superpowers/specs/2026-07-02-stryker-v3-ci-gate-design.mdpackages/stack/__tests__/v3-matrix/matrix-live-pg.test.tspackages/stack/package.jsonpackages/stack/src/encryption/v3.tspackages/stack/src/eql/v3/columns.tspackages/stack/src/eql/v3/table.tspackages/stack/src/schema/index.tspackages/stack/src/schema/match-defaults.ts
| pull_request: | ||
| branches: | ||
| - "**" | ||
| paths: | ||
| - 'packages/stack/src/eql/v3/**' | ||
| - 'packages/stack/src/schema/match-defaults.ts' |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Trigger the gate when the workflow itself changes.
push covers .github/workflows/fta-v3.yml, but pull_request does not. That means PRs can change this CI gate without ever running the updated workflow before merge.
Suggested fix
pull_request:
branches:
- "**"
paths:
- 'packages/stack/src/eql/v3/**'
- 'packages/stack/src/schema/match-defaults.ts'
+ - '.github/workflows/fta-v3.yml'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pull_request: | |
| branches: | |
| - "**" | |
| paths: | |
| - 'packages/stack/src/eql/v3/**' | |
| - 'packages/stack/src/schema/match-defaults.ts' | |
| pull_request: | |
| branches: | |
| - "**" | |
| paths: | |
| - 'packages/stack/src/eql/v3/**' | |
| - 'packages/stack/src/schema/match-defaults.ts' | |
| - '.github/workflows/fta-v3.yml' |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/fta-v3.yml around lines 21 - 26, Add the workflow file
itself to the pull_request path filters in fta-v3 so changes to
.github/workflows/fta-v3.yml also trigger the gate; update the pull_request
paths alongside the existing packages/stack/src/eql/v3 and schema/match-defaults
entries so the workflow validates its own changes before merge.
|
Multi-angle review (8 finder passes + verification, cross-checked against the branch, base, and main). This PR addresses the seven follow-ups from the #541 review, and all seven are faithfully and correctly implemented:
The findings below are all in the new code this PR introduces — none block merging, but 1–3 are worth fixing while the file is open. Findings1. 2. 3. 4. 5. The new unknown-table failure branch has no test. 6. 7. 8. Automated multi-agent review (8 finder angles, verified by direct inspection and a live CI-log check); merge compatibility with #542 verified via |
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Approved, with some findings that should be considered for a follow-up.
freshtonic
left a comment
freshtonic
left a comment
There was a problem hiding this comment.
Approving — all seven follow-ups from the #541 review are faithfully and correctly implemented, CI is green with the live-pg matrix genuinely running (40 tests), and the branch merges cleanly with #542.
Detailed feedback with 8 non-blocking findings is in #559 (comment) — the top three (identity-keyed reconstructor Map narrowing accepted tables, the FTA gate triggering on match-defaults.ts without scanning it, and the shared mutable unknownTableFailure object) are worth addressing while the file is open, but none block merging.
2 participants
Summary
Addresses the pending review follow-ups from #541 (merged) on the EQL v3 types module. Seven findings resolved across tests, schema, encryption, eql/v3, and CI.
Stacked on top of
feat/eql-v3-text-search-schema(#535).Changes
text_ord/text_ord_orecarry bothhm(unique) andob(ore), so the single-kind classifier ran only theeq_term/hmac_256proof and silently skippedord_term/ore_block_256— a wrong-valuedobwould pass the matrix green. Now runs both proofs; builds theobterm withqueryType:'orderAndRange'since equality resolves tohmon these domains.resolveMatchOpts()merge+clone helper used by both the v2 and v3freeTextSearchbuilders, giving v2 the clone-on-write protection it lacked (it stored caller opts by reference).EncryptedTextSearchColumn.build()'s unique/ore blocks viaindexesForCapabilities(override only match) so the emitted index shape cannot drift from the shared capability mapping.typedClientconstruction and return aDecryptionErrorfailure for unknown tables, sotable.build()'s duplicate-column throw can no longer escape the decryptResultcontract as a promise rejection.V3DecryptedModeltoV3ModelInput(character-identical) to stop the input/output model shapes silently drifting.src/schema/match-defaults.tsto the fta-v3 path scope — it shapes every emitted v3 match block but sat outside the gate.--score-cap 72 → 69after the eql/v3 file split and refresh the spec doc's stale71.08baseline claim.Review
CodeRabbit review against base
feat/eql-v3-text-search-schema: 0 findings.Test plan
hmandobproofs for text order domainsSummary by CodeRabbit