Repository navigation
refactor(sdk): simplify persisted finding workflows - #1210
Conversation
Repository fixtures and hydration tools duplicate Git process setup. Share the existing invocation patterns with their actual consumers.
SDK entrypoints and tests repeat runtime, authentication and option setup. Share the setup at existing owners and introduce internal helpers with their first consumers.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It broadly changes contract, persistence, recovery, and test infrastructure and remains dependent on the documented stacked-PR restacking process.
Review effort: Balanced
Findings: None
What changed in this PR
Consolidates persisted-finding, contract, recovery, and test-fixture workflows while preserving existing behavior.
Changes:
- Adds shared runtime, finding-entry, JSONL, MCP, fixture, and test helpers.
- Simplifies scan comparison, contract validation, deduplication, severity, and multiscan logic.
- Refactors persistence and recovery tests to reuse common fixtures and mocks.
| File | Description |
|---|---|
sdk/typescript/tests-ts/support/workflow-fixture.ts |
Reuses temporary-directory and scan-fixture helpers. |
sdk/typescript/tests-ts/support/mcp-client.ts |
Adds a shared MCP test client. |
sdk/typescript/tests-ts/support/json.ts |
Adds JSONL file reading. |
sdk/typescript/tests-ts/support/deduplication.ts |
Adds an empty-neighborhood reviewer fixture. |
sdk/typescript/tests-ts/suggest-owners.test.ts |
Reuses managed temporary directories. |
sdk/typescript/tests-ts/scan-resume.test.ts |
Reuses shell, JSONL, fixture, and failure helpers. |
sdk/typescript/tests-ts/scan-recovery.test.ts |
Consolidates recovery fixture setup. |
sdk/typescript/tests-ts/scan-recipe.test.ts |
Reuses test fixtures and throwing helpers. |
sdk/typescript/tests-ts/scan-matching-e2e.test.ts |
Simplifies hashing and fixture setup. |
sdk/typescript/tests-ts/scan-comparison.test.ts |
Consolidates mocks and temporary directories. |
sdk/typescript/tests-ts/project-config.test.ts |
Reuses temporary-directory fixtures. |
sdk/typescript/tests-ts/plugin-finding-detail-contract.test.ts |
Reuses the MCP client helper. |
sdk/typescript/tests-ts/multiscan.test.ts |
Consolidates mocks, JSONL parsing, and fixtures. |
sdk/typescript/tests-ts/knowledge-base.test.ts |
Centralizes temporary-directory tracking. |
sdk/typescript/tests-ts/import-scan.test.ts |
Reuses cleanup and rejection helpers. |
sdk/typescript/tests-ts/findings-server.test.ts |
Simplifies provider mocks. |
sdk/typescript/tests-ts/findings-dashboard.test.ts |
Simplifies timer and promise fixtures. |
sdk/typescript/tests-ts/findings-client-retry.test.ts |
Consolidates response and retry mocks. |
sdk/typescript/tests-ts/finding-workflow.test.ts |
Simplifies rejected workflow operations. |
sdk/typescript/tests-ts/finding-workflow-integration.test.ts |
Shares finding-service and mock helpers. |
sdk/typescript/tests-ts/finding-embeddings.test.ts |
Simplifies provider-call assertions. |
sdk/typescript/tests-ts/finding-deduplication.test.ts |
Consolidates candidate and reviewer fixtures. |
sdk/typescript/tests-ts/finding-catalogue.test.ts |
Shares evidence request builders. |
sdk/typescript/tests-ts/feedback.test.ts |
Reuses a throwing fixture. |
sdk/typescript/tests-ts/dedupe-records.test.ts |
Replaces counters with mocks and shared errors. |
sdk/typescript/tests-ts/contract.test.ts |
Reuses directories and call-count mocks. |
sdk/typescript/tests-ts/codex-review.test.ts |
Shares JSONL, directory, and validation helpers. |
sdk/typescript/tests-ts/classify-severity.test.ts |
Centralizes policy fixture cleanup. |
sdk/typescript/tests-ts/classify-scan-severity.test.ts |
Reuses scan and directory fixtures. |
sdk/typescript/src/value.ts |
Adds a finding-to-map-entry helper. |
sdk/typescript/src/severity-store.ts |
Reuses workbench runtime setup. |
sdk/typescript/src/server/sqlite-store.ts |
Reuses workbench runtime setup. |
sdk/typescript/src/scan-comparison.ts |
Simplifies options, evidence, and grouping logic. |
sdk/typescript/src/saved-scan.ts |
Reuses non-empty string validation. |
sdk/typescript/src/runtime.ts |
Adds shared workbench environment/runtime helpers. |
sdk/typescript/src/owner-evidence.ts |
Reuses missing-file handling. |
sdk/typescript/src/multiscan.ts |
Consolidates missing-file and attempt parsing logic. |
sdk/typescript/src/knowledge-base.ts |
Removes a redundant symlink branch. |
sdk/typescript/src/findings-import.ts |
Reuses the shared SHA-256 helper. |
sdk/typescript/src/finding-workflow.ts |
Reuses workbench environment construction. |
sdk/typescript/src/deduplication/scan.ts |
Simplifies repository and environment preparation. |
sdk/typescript/src/deduplication/records.ts |
Uses native set operations for attribution. |
sdk/typescript/src/deduplication/checkpointed-review.ts |
Inlines configuration collection. |
sdk/typescript/src/contract.ts |
Consolidates normalization and validation helpers. |
sdk/typescript/src/classify-severity.ts |
Reuses finding map entries. |
sdk/typescript/src/classify-scan-severity.ts |
Reuses workbench environment construction. |
sdk/typescript/scripts/smoke-findings-service.ts |
Reuses JSONL parsing in smoke checks. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Persisted finding workflows and recovery tests repeat owner calls and data preparation. Consolidate those paths while retaining current recovery and validation behavior.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed a8a76bab5dc1b0d76b80b9d10d780128b0f5da2e against its own base aa2b7867856b46440c6b671cb29f2f21a43b700d with exactly three independent high-reasoning full-diff reviews, including every test diff, plus root verification. No serious introduced code defect identified.
664 focused SDK tests passed (7 skipped); fresh native host build, plugin/SDK builds, types and formatting passed. A probe using the installed Codex SDK and a synthetic executable confirmed that absent and explicitly undefined Cyber feature values produce identical launch arguments; enabled values are retained.
Local validation was on macOS arm64. No model execution, local Windows run or full installed-package smoke. Reviewed sources remained unchanged. This review covers this exact head; it does not establish combined-stack validation against current main.
a8a76ba to
3bfbd7e
Compare
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
# Conflicts: # sdk/typescript/src/security-policy.ts # sdk/typescript/tests-ts/api-policy.test.ts # sdk/typescript/tests-ts/api-post-scan.test.ts # sdk/typescript/tests-ts/component-scan.test.ts # sdk/typescript/tests-ts/security-policy.test.ts
# Conflicts: # sdk/typescript/tests-ts/multiscan.test.ts
# Conflicts: # sdk/typescript/tests-ts/support/shell.ts
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed exact head 030d6b382fb1a9c7a6a9d9aeb2d52a018e5742d5 against 33746cd2e1766cf658fffa72d8adf7a4c6422828. Exactly three fresh independent HIGH full-diff reviews, including every test diff, plus root verification are complete. No serious introduced product finding remains.
Approval is withheld while inherited installed-package/container checks fail on the cleanup assertion described in #1207 (package-behavior.mjs:202). This is a base fixture/cleanup-contract failure at this head, not a new finding in this PR contribution. Correct it upstream and rerun hosted checks.
Local validation: 669 SDK tests passed,7 skipped. Fresh macOS arm64 native host, plugin and SDK builds, types and formatting passed. Reviewed sources are unchanged. No model execution, local Windows run or full installed-package smoke. Source review is complete; merge readiness is not established.
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Source review approved for a54f8bc546f959554daf6e0e504e40c4792535df against d519e01451b45a46ea2e9b6d3fd28489f8089d99.
Retained the three completed independent HIGH reasoning full-diff reviews, including every test diff, because this PR's own contribution is unchanged. Independently reconciled the current base and head snapshots and inherited changes in touched files. No additional full review pass was run for this restack, and no actionable introduced finding remains.
Validation is retained for the unchanged touched source: 669 distinct SDK tests passed and 7 were skipped. The suite was not rerun for this restack. The inherited package fixture was checked with the production tarball and existing exact-version dependencies; a clean installation remains unverified. This is not a new whole-tree validation claim.
Hosted CI was still pending at the latest check (2026-10-03T23:28:38.207458+00:00), with no failed jobs observed on this head. This source approval is not a statement that the PR is ready to merge. No model execution, local Windows validation, or full installed-package smoke test is claimed.
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Source review approved for b32fd7670343c50fa2129cfdfc6780cf69f826cc against dff043688e8e0d7cb9079525eec82ab61eed195f.
Retained exactly three independent HIGH full-diff review passes, including every test diff. Independently verified the actual GitHub three-dot diff, identical ordered contributions, all 52 touched paths at both base and head (bytes and modes), and the three historical report hashes. No actionable introduced source finding remains. No additional full source pass or local test was run for this restack; prior validation retains its original revision, execution time, and limitations.
Hosted CI was still unfinished at the separate 2026-10-04 05:14 UTC observation.
This is source approval, not a merge-readiness or combined-integration claim. No fresh model execution, local Windows validation, native rebuild, or clean installed-package smoke test is claimed.
Summary
Persisted finding workflows and recovery tests repeat owner calls and data preparation. Consolidate those paths while retaining current recovery and validation behavior.
Changes
Testing
Risk and rollout
Persistence, validation and recovery semantics remain unchanged. The progress and findings patches both touch internal value/JSON helpers and the same dashboard test reduction; their combined version must retain both helper sets and apply the test reduction once.
Targets
main, withdff04368merged. The original refactor contribution is unchanged; conflicts from the landed parent PRs are resolved.Direct follow-ups: #1211.
Public disclosure review