Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Tech debt#618

Open
pocesar wants to merge 2 commits into
Agent-Field:main from
pocesar:tech-debt
Open

Tech debt #618
pocesar wants to merge 2 commits into
Agent-Field:main from
pocesar:tech-debt

Conversation

@pocesar

@pocesar pocesar commented Jun 5, 2026

Copy link
Copy Markdown

Summary

Type of change

  • Bug fix
  • New feature
  • Refactor / cleanup
  • Docs only
  • Tests only
  • CI / tooling
  • Breaking change

Test plan

  • [ ]
  • [ ]

Test coverage

This repo enforces a coverage gate on every PR (see
.github/workflows/coverage.yml and
docs/COVERAGE.md).

  • The gate runs ./scripts/coverage-summary.sh and compares per-surface
    numbers against coverage-baseline.json.
  • It fails if any surface drops more than 1.0 pp below its baseline,
    if any surface is below 80%, or if the weighted aggregate falls below
    85%.

Before asking for review, please confirm:

  • I ran tests for the surface(s) I changed locally.
  • New code paths are covered by tests in this PR (no bare additions).
  • If I removed code, I updated coverage-baseline.json in
    this PR only if the removal caused a legitimate regression and I
    called it out in the summary above.
  • The coverage gate check is green in CI before requesting review.

For AI coding agents: if the Coverage Summary GitHub Actions job
fails, read the sticky PR comment titled "📊 Coverage report" — it lists
the specific surface that regressed, the delta, and the exact command to
reproduce locally. Add tests for the uncovered lines in this same PR
before requesting review again. Do not lower baselines to silence the
gate unless the regression is intentional and explicitly called out in
the PR summary.

Checklist

Related issues / PRs

@pocesar pocesar requested review from a team and AbirAbbas as code owners June 5, 2026 04:19

CLAassistant commented Jun 5, 2026 •
edited
Loading

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@santoshkumarradha santoshkumarradha left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

-- RetryStaleWorkflowExecutions increments this column when resetting stuck runs.

-- +goose Up
ALTER TABLE workflow_executions ADD COLUMN IF NOT EXISTS retry_count INTEGER NOT NULL DEFAULT 0;

@santoshkumarradha santoshkumarradha Jun 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This migration breaks the current SQLite test path. ALTER TABLE ... ADD COLUMN IF NOT EXISTS is not accepted by the SQLite version our suite uses, so go test ./... fails during storage initialization with near "EXISTS": syntax error. Please switch this to the same compatibility pattern we use in earlier migrations so both fresh SQLite DBs and already-migrated DBs work.

Comment thread compose.yaml
- from agent import agent;
agent.run()
volumes:
- ./agent.py:/app/agent.py

@santoshkumarradha santoshkumarradha Jun 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This compose example points at ./agent.py, but there is no agent.py at the repo root. In a fresh checkout the bind mount source is missing, so the agent and team services cannot start as written. If this is meant to reuse the triggers demo, please wire it to the real file under examples/triggers-demo/ or add the missing entrypoint alongside this compose file.

github-actions Bot commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

Performance

SDK Memory Δ Latency Δ Tests Status
TS 354 B +1% 1.41 μs -30% ✓ ✓

✓ No regressions detected

Copy link
Copy Markdown
Member

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:

  • The new storage migration breaks fresh SQLite/local startup (migration 016: near "EXISTS": syntax error), and it also appears to add then drop retry_count in the embedded migration.
  • The root compose.yaml hardcodes postgres/postgres, abc123, and admin123, exposes ports, and references a root agent.py that does not exist.
  • Telemetry/docs changes are not just cleanup for us. Anonymous OSS telemetry and its public disclosure are critical, so flipping telemetry off and removing the docs/privacy context needs a dedicated discussion.
  • The new frontend cpClient should not attach API/admin tokens to arbitrary absolute URLs; it should stay same-origin/path-only before it becomes a shared helper.
  • Identity/DID/VC and admin gRPC removals are security/audit surface changes, not tech debt, and need their own PR with rationale and replacement plan.

I’d recommend reducing this into small focused PRs: .DS_Store cleanup, migration fix, compose example, frontend API client refactor, and any identity/admin removals separately.

pocesar commented Jun 6, 2026

Copy link
Copy Markdown
Author

@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 😆
santoshkumarradha reacted with laugh emoji

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@santoshkumarradha santoshkumarradha santoshkumarradha requested changes

@AbirAbbas AbirAbbas Awaiting requested review from AbirAbbas AbirAbbas is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /