Skip to content

chore: add v3 cleanup plan for CLI and server #745

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
jbpros wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

chore: add v3 cleanup plan for CLI and server #745

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5c1bbb1
chore: add v3 cleanup plan for CLI and server
jbpros Mar 30, 2026
eb54dc3
chore: align v3 cleanup plan with kosli-dev/server#5132
jbpros Mar 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
161 changes: 161 additions & 0 deletions TODO.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,167 @@
<!-- Each feature gets its own ## section below. -->
<!-- Only edit YOUR feature's section. Delete it after merging to main. -->

## CLI v3 — Breaking Changes & Cleanup

Tracking issue: kosli-dev/server#5132

### Deprecated Commands to Remove

| Command | Replacement | File |
|---------|-------------|------|
| `kosli report artifact` | `kosli attest` commands | `cmd/kosli/reportArtifact.go` |
| `kosli snapshot server` (alias `directories`) | `kosli snapshot paths` | `cmd/kosli/snapshotServer.go` |
| `kosli report approval` | Attestation workflow | `cmd/kosli/reportApproval.go` |
| `kosli request approval` | Attestation workflow | `cmd/kosli/requestApproval.go` |
| `kosli get approval` | Attestation workflow | `cmd/kosli/getApproval.go` |
| `kosli list approvals` | Attestation workflow | `cmd/kosli/listApprovals.go` |
Comment on lines +16 to +19
Copy link
Copy Markdown
Contributor

@claude claude bot Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approval commands (report approval, request approval, get approval, list approvals) are not currently marked as deprecated in their cobra command definitions — no Deprecated: field is set on any of them. This is different from report artifact and snapshot server which are formally deprecated.

The plan should note this: these commands need deprecation warnings added in a v2.x release first (so users get advance notice), or the plan should explicitly state they'll be removed without a deprecation period in v3.


### Deprecated Flags to Remove

| Flag | Command | Replacement | File |
|------|---------|-------------|------|
| `--visibility` | `create flow` | Remove (org-level concern, not flow-level) | `cmd/kosli/createFlow.go` |
Copy link
Copy Markdown
Contributor

@claude claude bot Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: --visibility is not currently marked as deprecated in createFlow.go (no DeprecateFlags or MarkDeprecated call for it). Same for --template — it's used in the legacy code path but never formally deprecated. Worth noting this distinction: these flags need to be deprecated (with a warning in v2.x) before removal in v3, or the plan should clarify they'll be removed without a deprecation period.

Copy link
Copy Markdown
Contributor

@claude claude bot Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same note as the previous review: --visibility is not formally deprecated — there's no DeprecateFlags or MarkDeprecated call for it in createFlow.go. Same for --template. Both are still actively registered flags.

Consider adding deprecation warnings for these in a v2.x release before removing them in v3, or document that they'll be removed without a deprecation period.

| `--registry-provider` | fingerprint commands | None ("no longer used") | `cmd/kosli/flags.go` |
| `--require-provenance` | `create environment` | Policies | `cmd/kosli/createEnvironment.go` |
| `--cluster` / `-C` | `snapshot ecs` | `--clusters` | `cmd/kosli/snapshotECS.go` |
| `--service-name` / `-s` | `snapshot ecs` | `--services` | `cmd/kosli/snapshotECS.go` |
| `--function-name` | `snapshot lambda` | `--function-names` | `cmd/kosli/snapshotLambda.go` |
| `--function-version` | `snapshot lambda` | None (non-functional, kept for compat) | `cmd/kosli/snapshotLambda.go` |
| `-e` (exclude shorthand) | `fingerprint`, `snapshot server` | `-x` | `cmd/kosli/fingerprint.go`, `cmd/kosli/snapshotServer.go` |

### Flow Creation Simplification

- [ ] Remove the `--template` string-slice flag and the legacy code path in `createFlow.go` (lines 138–153)
Copy link
Copy Markdown
Contributor

@claude claude bot Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The line numbers referenced here (138–153) are approximate and will drift as the file changes. Consider referencing the code by its identifying characteristics instead (e.g., "the if o.TemplateFile == "" && !o.UseEmptyTemplate branch") so this plan stays accurate over time.

(Echoing the previous review's feedback since it still applies after the latest commit.)

- [ ] Remove `--use-empty-template` flag — no template flag should now mean "no template" (no auto-injected "artifact")
- [ ] Remove `injectArtifactIntoTemplateIfNotExisting()` — the silent default of adding "artifact" goes away
- [ ] Only keep `--template-file` path, which hits `PUT /api/v2/flows/<org>/template_file`
- [ ] Remove the deprecated server endpoint `PUT /api/v2/flows/<org>` (see Legacy Flow Creation Endpoint below)

### Richer Exit Codes

- [ ] Implement structured exit codes (see kosli-dev/server#4988, kosli-dev/server#3728, and [CLI PR #714](https://github.com/kosli-dev/cli/pull/714) with ADR)
- [ ] `kosli evaluate` must distinguish "policy denied" from "something went wrong" (currently both exit 1)

### Terminology: "evidence" → "attestation"

Update user-facing flag descriptions and help text that still say "evidence":
- [ ] `commitPREvidenceFlag` in `root.go` — "find pull request evidence" → "find pull request attestation"
- [ ] `resultsDirFlag` in `root.go` — "evidence vault" → "attestation vault" (or just "vault")
- [ ] `snykJsonResultsFileFlag` in `root.go` — "evidence vault"
- [ ] `snykSarifResultsFileFlag` in `root.go` — "evidence vault"
- [ ] `attachmentsFlag` in `root.go` — "evidence vault"
- [ ] Internal variable names: `evidencePath`, `evidencePaths` in attestation code (`attestation.go`, `pullrequest.go`, `attestCustom.go`, `attestGeneric.go`, `attestSnyk.go`, `attestSonar.go`)
Copy link
Copy Markdown
Contributor

@claude claude bot Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: renaming internal variable names (evidencePath, evidencePaths) is a good cleanup but isn't a breaking change. Consider separating this section into "breaking" (flag/help text changes visible to users) and "non-breaking" (internal renames) to help prioritize. Internal renames could land in v2.x without waiting for v3.

- [ ] Test descriptions in `assertPRGithub_test.go` — "PR evidence"

### Command Aliases — Review (discuss with team)

- [ ] `begin trail` aliases: `start`, `init` — keep all?
- [ ] `attest pr` aliases: `pr`, `mr`, `mergerequest`, `pullrequest` — keep all?
- [ ] `environment` aliases: `env`, `envs` — keep all?
- [ ] `snapshot kubernetes` alias: `k8s` — keep?
- [ ] `snapshot s3` alias: `S3` — keep?

### API Compatibility Shim in `getArtifact.go`

- [ ] Remove `printArtifactAsTableWrapper()` — exists because the API returns an array for commit queries but a map for sha256 queries
- [ ] Fix API to consistently return an array, then remove the wrapper

### Config File Backward Compatibility

- [ ] Remove old default config location fallback in `root.go` (comment: "for backward compatibility with old default config location")

### Legacy Flow Handling in `assertArtifact.go`

- [ ] Remove legacy flow detection code (lines 220–221) that checks for "legacy_flow" and skips new attestation logic
Copy link
Copy Markdown
Contributor

@claude claude bot Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same fragile line-number reference (lines 220–221). The actual code is a case "legacy_flow": branch in a switch statement — that description would be more durable.

(Echoing the previous review since it still applies.)

- [ ] Note: the `legacy_flow` resolution type in the server (`src/model/policy_compliance_checker.py`) is **defined but never created** — it's dead code on both sides
Comment on lines +76 to +78
Copy link
Copy Markdown
Contributor

@claude claude bot Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same fragile line-number reference here (lines 220–221). The actual code is a case "legacy_flow": branch in a switch statement — that description would be more durable.


### Tests & Docs

- [ ] Remove all test cases that verify deprecation warnings for removed commands/flags
- [ ] Update golden files / test output expectations
- [ ] Update doc generation if deprecation handling changes

---

### Server-Side Cleanup

#### Flask-to-FastAPI Migration Infrastructure

- [ ] Remove `RegexDispatcherMiddleware` (`src/fastapi_app/middleware.py`) — marked with `FASTAPI-POST-MIGRATION-CLEANUP` comment
- [ ] Remove bearer token prepending workaround (middleware lines 68–73)
- [ ] Remove PR attestation test-environment special case (middleware lines 25–34)
- [ ] Remove `is_api_path_migrated` / `is_public_resource` helpers (`src/auth/register_check_route_access.py`)
- [ ] Remove `migrated_apis` list and `api-migration-index` feature flag logic (`src/lib/feature_flags.py`)

#### Legacy Flow Creation Endpoint

- [ ] Remove deprecated `PUT /api/v2/flows/<org>` endpoint (Flask: `src/apis/v2/flow.py`, FastAPI: `src/fastapi_app/v2/flow.py`)
- [ ] Remove `LegacyCreateFlow` model and schema (`src/apis/schemas/flows/flows_declare_v2.json`)
- [ ] Remove `FlowTemplateConverter` (`src/model/flow_template_converter.py`) — only needed for inline-to-YAML conversion

#### V1 Flow Migration Code

- [ ] Remove migration scripts in `src/migrations/completed_migrations/migrate_v1_flows.py` and related undo/verify scripts (once all orgs confirmed migrated)
- [ ] Remove `has_trail` and `is_in_migration` properties from artifact models
- [ ] Remove `_legacy_compliance_state()` method and legacy status calculations

#### Flow Visibility Field

- [ ] Remove `visibility` from the flow model (`src/model/flow.py`) — access control is org-level only
- [ ] Remove `visibility` from flow creation payloads and API schemas
- [ ] Consider a migration to drop the field from existing flow documents

#### Rename `pipeline_id` → `flow_id` Throughout

- [ ] Rename `pipeline_id` to `flow_id` in artifact model (`src/model/artifact.py`), attestation model, trail model
- [ ] Remove `pipeline_name` → `flow_name` converter in FastAPI Pydantic models (`src/fastapi_app/models/artifacts.py`)
- [ ] Update database indexes in `src/documentdb/indexes.py` (many still reference `pipeline_id`)
- [ ] Requires a data migration for existing documents
Comment on lines +116 to +121
Copy link
Copy Markdown
Contributor

@claude claude bot Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the highest-risk item in the plan — a cross-cutting rename with a required data migration. Consider calling out an explicit sequencing constraint: the data migration must land and complete before any code referencing flow_id is deployed, or both field names need to be supported during a transition window. This deserves its own slice/epic rather than being a bullet in a larger list.

Comment on lines +117 to +121
Copy link
Copy Markdown
Contributor

@claude claude bot Mar 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the highest-risk item in the plan — a cross-cutting rename with a required data migration. Consider calling out an explicit sequencing constraint: the data migration must land and complete before any code referencing flow_id is deployed, or both field names need to be supported during a transition window. This deserves its own slice/epic rather than being a bullet in a larger list.

(Echoing the previous review since it still applies and is critical.)


#### Deprecated Event Types

- [ ] Remove legacy `started` and `changed` event types (`src/model/environment_consts.py`)
- [ ] Only keep the specific types: `started-compliant`, `started-non-compliant`, `started-unknown`, `became-compliant`, `became-non-compliant`, `scaled`, `updated-provenance`
- [ ] Update display priority logic and event validation

#### Legacy Attestation Types

- [ ] Remove `pull_request_legacy` attestation type support (`src/model/attestations_model/attestation.py`)
- [ ] Remove `generic_legacy` type conversion logic
- [ ] Update compliance checker to remove legacy type handling (`src/model/compliance_checker.py`)

#### Dead Code: `legacy_flow` Resolution Type

- [ ] Remove `legacy_flow` from `RuleResolution` type in `src/model/policy_compliance_checker.py` — defined but never created anywhere

#### Expect Deployment Remnants

- [ ] Remove deployment feature flag definitions in `src/lib/feature_flags.py` (indices 38–40)
- [ ] Review whether `ArtifactDeploymentEvent` in `src/model/trail_events.py` is still needed or can be cleaned up
- [ ] Clean up any orphaned deployment data models if no longer referenced by active code

#### Backward Compatibility Shims

- [ ] Remove deprecated `interval` parameter for environment snapshots (`src/fastapi_app/common/environment.py`) — replaced by `start_index`/`end_index`
- [ ] Remove `parse_snapshot_interval()` function
- [ ] Remove `legacy_approval_format` conversion code (`src/model/approvals.py`)

#### Authentication: Userfront Removal

- [ ] Remove `legacy_userfront_signin_page()` fallback (`src/auth/register_descope_auth.py`)
- [ ] Remove Userfront-related feature flags (`is-domain-using-descope-sso`, `is_using_descope`, `is-descope-session-validation-enabled`)
- [ ] Consolidate auth to Descope only

#### Experimental Features Flag

- [ ] Review `experimental_features_enabled` org field and `/api/v2/organizations/{org}/experimental_features` endpoint — remove if all orgs migrated

#### Legacy Snapshot Test Helpers

- [ ] Remove `test/helpers/unit/lib/legacy_k8s_snapshots.py` and legacy v1 snapshot test parametrization

---

## kosli evaluate trail

- [x] Slice 1: Skeleton `evaluate` parent + `evaluate trail` fetches trail JSON
Expand Down
Loading