Repository navigation
refactor(plugin): simplify workbench owner calls - #1205
Conversation
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. |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The refactor preserves existing behavior, updates affected probes and tests, and introduces no identified blocking issues.
Review effort: Balanced
Findings: None
What changed in this PR
Simplifies Python workbench ownership by removing forwarding functions, consolidating equivalent helpers, and updating dependent tests and SDK probes.
Changes:
- Calls database, validation, Deep Scan, and filesystem owners directly.
- Consolidates repeated test fixtures and collection logic.
- Updates probes for explicit Deep Scan dependency configuration.
| File | Description |
|---|---|
sdk/typescript/tests-ts/workbench-canonical-paths.test.ts |
Configures synthetic Deep Scan dependencies. |
sdk/typescript/tests-ts/support/workbench-command.ts |
Adds a shared workbench test command. |
sdk/typescript/tests-ts/deep-scan-workbench.test.ts |
Uses shared command and explicit configuration. |
sdk/typescript/tests-ts/deep-scan-timestamp-compat.test.ts |
Configures timestamp probe dependencies. |
plugins/codex-security/tests/workbench_test_support.py |
Adds shared loaders and fixtures. |
plugins/codex-security/tests/test_workbench_timestamps.py |
Overrides the dependency owner directly. |
plugins/codex-security/tests/test_workbench_db_exports.py |
Calls saved-result ownership directly. |
plugins/codex-security/tests/test_windows_scope_streams.py |
Patches the stream-check owner. |
plugins/codex-security/tests/test_windows_scan_local_files.py |
Uses the shared script loader. |
plugins/codex-security/tests/test_windows_report_e2e.py |
Uses the shared Windows backend fixture. |
plugins/codex-security/tests/test_validate_tracking_source.py |
Shares scan fixtures and standard hashing. |
plugins/codex-security/tests/test_validate_scan_contract.py |
Shares validator fixtures and hashing. |
plugins/codex-security/tests/test_sarif_projection.py |
Uses the shared script loader. |
plugins/codex-security/tests/test_report_projection.py |
Removes duplicate loading logic. |
plugins/codex-security/tests/test_finalize_scan_contract.py |
Reuses shared fixtures and backend behavior. |
plugins/codex-security/tests/test_capability_profiles.py |
Consolidates fallback tests. |
plugins/codex-security/scripts/workbench_validation.py |
Delegates validation to existing owners. |
plugins/codex-security/scripts/workbench_target.py |
Removes unused forwarding helpers. |
plugins/codex-security/scripts/workbench_target_state.py |
Simplifies standalone argument handling. |
plugins/codex-security/scripts/workbench_source_excerpt.py |
Simplifies standalone argument handling. |
plugins/codex-security/scripts/workbench_schema.py |
Simplifies standalone argument handling. |
plugins/codex-security/scripts/workbench_scan_usage.py |
Consolidates counting and deduplication. |
plugins/codex-security/scripts/workbench_scan_start.py |
Calls text normalization directly. |
plugins/codex-security/scripts/workbench_remediation.py |
Simplifies standalone argument handling. |
plugins/codex-security/scripts/workbench_progress.py |
Inlines UTF-16 length calculation. |
plugins/codex-security/scripts/workbench_native_indexes.py |
Uses Counter for scan totals. |
plugins/codex-security/scripts/workbench_db.py |
Removes wrappers and calls owners directly. |
plugins/codex-security/scripts/workbench_constants.py |
Owns shared positive-integer parsing. |
plugins/codex-security/scripts/workbench_cli.py |
Imports shared integer parsing. |
plugins/codex-security/scripts/windows_scan_local_files.py |
Simplifies standalone argument handling. |
plugins/codex-security/scripts/report_projection.py |
Removes redundant projection wrappers. |
plugins/codex-security/scripts/rank_preview.py |
Uses insertion-ordered dictionaries for uniqueness. |
plugins/codex-security/scripts/generate_rank_input.py |
Consolidates validation and assignment handling. |
plugins/codex-security/scripts/finalize_scan_contract.py |
Removes hashing and validation forwarders. |
plugins/codex-security/scripts/filesystem_identity.py |
Simplifies standalone argument handling. |
plugins/codex-security/scripts/deep_scan_workbench.py |
Calls configured dependencies directly. |
plugins/codex-security/scripts/config_preflight.py |
Reuses shared integer parsing. |
plugins/codex-security/mcp-app/src/python_command.ts |
Simplifies executable stat checking. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
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". |
The Python workbench carries forwarding functions and repeated data handling around existing owners. Call those owners directly and consolidate equivalent operations.
alandelong-oai
left a comment
alandelong-oai
left a comment
There was a problem hiding this comment.
Reviewed 410e85b0e97b5c27ea931aa343653e786654eb07 against its own base 3642a76a53173e012f579737d1b6cf4096379af2 with exactly three independent high-reasoning full-diff reviews, including every test diff, plus root verification. No serious introduced code defect identified.
51 focused SDK tests passed (2 skipped); 336 Python tests and 66 subtests passed (2 skipped). Fresh native host build, plugin/SDK builds, types, formatting, Ruff and portable checks passed. An optional pytest-plugin cleanup callback emitted a sandbox process-list denial after the successful test run.
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.
GitHub currently reports a merge conflict against the updated base branch. Any changed patch from conflict resolution needs a new review.
410e85b to
4f70ecc
Compare
|
Codex Review: Didn't find any major issues. Bravo. 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.
Approved reviewed head 4f70ecc3cbaf6f002363f11eed4f526dfbcdc801 against 775e48ffbf682075e60303eb019131aa2390c3b2.
All 38 files have the same ordered added/removed PR contributions as the previously approved head. I retained the three independent full-diff reviews, including every test diff, and separately inspected the six inherited touched-file changes from #1133. No serious introduced findings remain.
Fresh validation at this head passed: 51 SDK tests with 2 skipped; 352 Python tests plus 66 passing subtests with 2 skipped; 9 portable source tests; native host, plugin and SDK builds; types, formatting, Ruff 0.16.9 and source compatibility. All 38 reviewed source files still match the head. Python logged sandbox permission diagnostics but exited successfully. Local macOS arm64 only; no model execution, source edits, installed-package smoke or local Windows run.
Hosted node-ci37152833472 attempt1 is still running. This approval covers the reviewed source; it does not assert completed CI or merge readiness.
Summary
The Python workbench carries forwarding functions and repeated data handling around existing owners. Call those owners directly and consolidate equivalent operations.
Changes
Testing
On rebased head
4f70ecc3, 699 tests and 110 subtests passed, with two skips, across 12 Python regression modules covering finalization, saved exports, completion binding, standard/deep results, and threat-model projection. Ruff check/format, SDK CI compilation, portable plugin source checks and tests, and MCP types passed. The rebase preserves all seven new threat-model helper implementations.Earlier combined-source validation passed the full Python suite (1,520 tests and 109 subtests, six skips) and seven SDK workbench owners (59 tests, one platform skip). Those complete suites were not rerun on this rebased head. Fresh hosted checks are pending.
Risk and rollout
The removal includes current caller closure. Synthetic probes must configure the provider they override; production initialization continues to configure Deep Scan before dispatch.
This patch has no source prerequisite within the series. Squash-merge it into
mainafter review and checks pass.Public disclosure review