Tech debt#618
Conversation
santoshkumarradha
left a comment
There was a problem hiding this comment.
Thanks for pushing this cleanup through. I found a couple of concrete regressions while validating locally, so I’m requesting changes before this can land.
I ran cd control-plane && go test ./... in a fresh checkout of this PR and the run fails during SQLite setup because the new migration uses IF NOT EXISTS on ALTER TABLE ... ADD COLUMN, which SQLite does not support in the version our tests exercise. I also checked the new root compose.yaml, and the agent services point at ./agent.py, but this repo does not have that file at the root, so the example stack will not start as written.
Also worth noting: I only see the CLA check on the PR right now, so the normal CI gates do not appear to have run yet.
Performance
✓ No regressions detected |
|
Thanks for the cleanup pass. I did a security-focused read because this PR touches a lot of sensitive surfaces. I don’t see evidence of a supply-chain/backdoor style issue, but I think this should be split up and fixed before merge. Main blockers:
I’d recommend reducing this into small focused PRs: |
|
@santoshkumarradha sorry about the sloppy PR, I was in a rush yesterday. I'll split them properly. Rest assured it's not an account takeover 😆 |
…onfig Salvage of the safe, zero-regression subset of Agent-Field#618. Removes genuinely dead code only: - control-plane/internal/handlers/ui/identity.go (+2 tests): the DID Explorer / Credentials UI backend. Its sole frontend consumer (identityApi.ts) has no callers, and the DID Explorer pages were already removed — App.tsx redirects /identity/dids and /identity/credentials to /settings. No live consumer. - web/client/src/services/identityApi.ts (+ test): orphaned frontend service. - control-plane/config/docker-perf.yaml: unreferenced by any Makefile/CI/compose. Deliberately EXCLUDES the regression-inducing parts of the original PR: the /admin/public-key alias removal (breaks all-SDK offline VC verification), node lifecycle + /actions/claim endpoints, legacy reasoner execute endpoints, the broken root compose.yaml, and storage-mode/telemetry config flips. Validation: go build/vet clean; go test ./... green (control-plane); web-ui npm build clean; web-ui coverage 84.78% (baseline 84.79%, floor 84.0). Co-authored-by pocesar via original PR Agent-Field#618. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Picked this back up since it had been idle for a while and was conflicting with main. Rather than land the full change, I've reduced it to the subset that's a clean, zero-regression cleanup and rebased it onto current main (it's now mergeable): Kept (genuinely dead code, ~1.5k lines):
Left out because they'd be regressions and deserve their own focused PRs:
Thanks for the original cleanup pass @pocesar — credited on the commit. |
📊 Coverage gateThresholds from
✅ Gate passedNo surface regressed past the allowed threshold and the aggregate stayed above the floor. |
📐 Patch coverage gateThreshold: 80% on lines this PR touches vs
✅ Patch gate passedEvery surface whose lines were touched by this PR has patch coverage at or above the threshold. |
Summary
Type of change
Test plan
Test coverage
This repo enforces a coverage gate on every PR (see
.github/workflows/coverage.ymlanddocs/COVERAGE.md)../scripts/coverage-summary.shand compares per-surfacenumbers against
coverage-baseline.json.if any surface is below 80%, or if the weighted aggregate falls below
85%.
Before asking for review, please confirm:
coverage-baseline.jsoninthis PR only if the removal caused a legitimate regression and I
called it out in the summary above.
Checklist
docs/DEVELOPMENT.md.
Related issues / PRs