# Engineering Review: Trae Workspace Tool P0 Plan

Review date: 2026-06-10

Reviewed documents:

- `docs/specs/trae-workspace-tool.md`
- `docs/plans/trae-workspace-tool-p0-execution-plan.md`

## Verdict

The plan is directionally sound and implementable. The main risk is not technical difficulty; it is doing too much during the rename. Keep P0 constrained to package rename, static profile/pack registry, one enterprise profile, one ops pack, doctor compliance checks, and dry-run semantics.

Do not introduce plugin loading, custom pack files, JSON doctor output, or multiple role packs in the same change.

## Findings

### P1: Package rename can create avoidable breakage

The spec requires renaming the project, package, and CLI. That is correct for product positioning, but it can break existing users and tests at the same time as the architecture changes.

Recommendation:

- Expose only `trae-workspace-tool` as the supported CLI command.
- Keep implementation in `trae_workspace_tool`.
- Add tests and smoke coverage for the new CLI.

Status:

- Addressed in the spec and execution plan by documenting the single supported CLI command.

### P1: Pack abstraction must stay small

The product direction needs multiple packs, but P0 only ships `ops`. A plugin framework would be over-engineered and would distract from the migration.

Recommendation:

- Use static in-process registries.
- Use dataclasses for `Asset`, `Profile`, and `Pack`.
- Avoid dynamic imports, external config parsing, remote registries, and user-authored pack loading in P0.

Status:

- Addressed in the architecture section.

### P1: Doctor compliance checks can become brittle

Moving from existence checks to compliance checks is valuable, but naive substring checks can produce false confidence or false failures.

Recommendation:

- P0 validators should check only stable invariants:
  - Rules contain `alwaysApply: true`.
  - Skills contain `name:`.
  - Generated scripts contain root containment and default dry-run markers.
- Defer deeper semantic validation to P1.

Status:

- Addressed in doctor requirements and rollback strategy.

### P1: Dry-run semantics must be settled before role expansion

The project safety promise depends on default-safe generated scripts. If `--dry-run` remains required for safety, the enterprise positioning is weaker.

Recommendation:

- Make dry-run the default.
- Keep `--dry-run` as an accepted compatibility no-op.
- Do not add operational `--execute` behavior in P0.

Status:

- Addressed in spec, plan, and test requirements.

### P2: Enterprise profile and ops pack layering needs deterministic ordering

The generated workspace will include both enterprise-wide safety rules and ops-specific rules. If ordering or ownership is unclear, future packs may duplicate or override policy.

Recommendation:

- Treat profile assets as baseline policy.
- Treat pack assets as role-specific additions.
- If asset paths collide, fail fast with a clear error rather than choosing one silently.

Status:

- Partially addressed. The implementation should add a selected-asset flattening test that fails on duplicate relative paths.

### P2: The plan should not silently defer distribution forever

The product target is enterprise intranet usage and later department rollout. P0 can defer PyPI and CI publishing, but installation and environment recovery must be clear.

Recommendation:

- P0 docs should include source install and local validation instructions.
- Rebuilding `.venv` should be part of P0 acceptance because the current one is wrong-architecture.
- Department distribution can stay P1/P2.

Status:

- Addressed in Phase 0 and documentation tasks.

### P2: Rename and behavior changes should be staged in small commits

The implementation touches package names, CLI behavior, generated templates, doctor behavior, and docs. Combining all changes in one unstructured diff will make review harder.

Recommendation:

Suggested commit sequence:

1. Rename package and CLI, preserving existing behavior.
2. Introduce registry model with default ops output unchanged.
3. Add `internal-basic` profile.
4. Upgrade doctor validators.
5. Fix maintenance script dry-run semantics.
6. Update README and docs.

Status:

- Add this sequence to implementation issue or PR checklist before coding.

## Test Diagram

```text
P0 CHANGE AREA                         REQUIRED TEST COVERAGE
Package rename
  ├── import new package                unit import path
  ├── new CLI command                   smoke
  └── old alias if retained             smoke

Profile/pack registry
  ├── known profile                     unit
  ├── known pack                        unit
  ├── unknown profile                   unit
  ├── unknown pack                      unit
  └── duplicate asset path              unit

Install flow
  ├── default profile+pack              unit
  ├── explicit profile+pack             unit
  ├── no-overwrite                      unit
  └── force overwrite                   unit

Doctor flow
  ├── all assets valid                  unit + smoke
  ├── missing asset                     unit
  ├── invalid rule                      unit
  ├── invalid skill                     unit
  └── invalid script marker             unit

Generated script
  ├── default dry-run                   integration
  ├── explicit --dry-run                integration
  ├── invalid root                      integration
  ├── custom dirs                       integration
  └── excludes                          integration
```

## Performance Review

No material performance risk in P0. Asset generation is bounded by a small static registry and local file writes. Keep validators linear over selected assets and avoid scanning target project trees outside generated file paths.

## Security Review

The plan improves security posture by adding enterprise-wide policy and default dry-run semantics. The main security requirement is to avoid implying enforcement that does not exist. Rules and Skills guide Trae behavior, but they do not sandbox the operating system.

Documentation should state:

- The tool generates guardrails; it is not a security boundary.
- Generated scripts must remain safe by construction.
- Human approval is required before any future real execution mode.

## Implementation Tasks

- [ ] **T1 (P1, human: ~1h / CC: ~10min)** - Rename - Add compatibility strategy for old CLI alias.
  - Surfaced by: Package rename risk.
  - Verify: CLI smoke for new command and old alias if retained.
- [ ] **T2 (P1, human: ~2h / CC: ~20min)** - Registry - Implement static profile/pack registry with duplicate asset path detection.
  - Surfaced by: Pack abstraction and layering risk.
  - Verify: registry unit tests.
- [ ] **T3 (P1, human: ~1h / CC: ~10min)** - Doctor - Add stable validators for Rule, Skill, and script safety markers.
  - Surfaced by: Doctor compliance risk.
  - Verify: corrupt generated files and assert doctor fails.
- [ ] **T4 (P1, human: ~1h / CC: ~10min)** - Script - Make maintenance checker dry-run by default.
  - Surfaced by: Safety semantics.
  - Verify: generated script tests without `--dry-run`.
- [ ] **T5 (P2, human: ~45min / CC: ~10min)** - Docs - Document source install, venv rebuild, and enterprise intranet usage.
  - Surfaced by: Distribution readiness.
  - Verify: README command examples run locally.

## Review Summary

- Scope challenge: accepted as constrained P0.
- Architecture review: 3 issues found, all addressed or assigned.
- Code quality review: 2 issues found, both addressed through small-registry and staged-commit recommendations.
- Test review: coverage diagram produced, 1 added gap identified for duplicate asset paths.
- Performance review: no issues found.
- Security review: 1 documentation caveat added.
- NOT in scope: written in spec and plan.
- What already exists: written in plan.
- Parallelization: sequential implementation recommended.
