Skip to content

feat(api): send the run environment on every v2 reconcile - #597

Open
theCodeDrift wants to merge 2 commits into
mainfrom
cli/reconcile-run-environment
Open

theCodeDrift wants to merge 2 commits into
mainfrom
cli/reconcile-run-environment

Conversation

@theCodeDrift

Copy link
Copy Markdown
Member

Every POST /cli/api/v2/reconcile now carries environment: { ci, provider? }, so the service can record when a repository last reconciled from CI. That is what the dashboard's "CI set up" indicator (taskless/taskless#334, item 3) waits on. Verdicts, entitlements and output are unchanged; the field never affects them.

What it sends

  • Off CI: { ci: false }.
  • On a recognised provider: { ci: true, provider: "github-actions" } (also gitlab-ci, bitbucket-pipelines, …).
  • On CI with an unrecognised provider: { ci: true }. The [unknown] / [none] telemetry sentinels are never sent; they exist to keep aggregates countable, and on the wire they would be values the service has to know to ignore.

The detection is the one telemetry already uses (isContinuousIntegration, resolveCiProvider), so the two can never disagree about whether a run was CI. Only the spelling differs: the published schema's example is hyphenated, while telemetry keeps the underscores its dashboards group by.

It is attached inside reconcileRules rather than by each caller, so neither current call site (check's plan, rules recover) nor a future one can send a reconcile without it.

Why the vendored schema was not regenerated

The live /cli/api/v2/__schema already publishes environment as optional, with additionalProperties: false on { ci?, provider? }. Regenerating to pick it up would also vendor three error codes the service has added since (github_forbidden, github_unavailable, cli_upgrade_required), and the exhaustive code lists in src/api/v2.ts then fail typecheck in seven places until each client decides how to handle them. That deserves its own change, so the field is declared as a local intersection on the request type with a comment saying it can be deleted once the schema is re-vendored.

Verified

  • New wire test asserts the exact body on CI (github-actions) and off it (ci: false), with the environment stubbed both ways so it does not depend on where the suite runs. It fails with the client change removed.
  • Unit cases for resolveRunEnvironment: local, CI=false beside a provider marker, recognised providers, and an unrecognised one.
  • pnpm typecheck, pnpm lint, and the full CLI suite (2839 passed, 10 skipped).
  • Not run against the live service; the body was checked against the live schema.

Refs #596

Reconcile now carries environment: { ci, provider? }, from the CI detection telemetry already performs. The service records when a repository last reconciled from CI, for the dashboard's CI indicator; verdicts are unchanged. Declared locally until the vendored v2 schema is re-vendored.
@theCodeDrift

Copy link
Copy Markdown
Member Author

@claude /review

@github-actions

github-actions Bot commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @theCodeDrift's task in 1m 9s —— View job


Review mode: incremental — read 0 prior review thread(s) before reviewing.

.prior-review.json contained no review threads, review summaries, or prior top-level comments (only the @claude /review trigger itself) — so this is the first review of this PR. All findings below are assessed fresh against the whole diff.

  • Read .prior-review.json (empty — no prior review state)
  • Fetch PR diff and changed files (gh pr diff)
  • Review packages/cli/src/api/v2.ts
  • Review packages/cli/src/util/adoption-dimensions.ts
  • Review tests (adoption-dimensions.test.ts, api-v2.test.ts)
  • Post final review

Assessment

This is a clean, well-scoped change. Highlights:

  • resolveRunEnvironment (packages/cli/src/util/adoption-dimensions.ts:166-175) correctly reuses isContinuousIntegration/resolveCiProvider so wire reporting and telemetry can never disagree on CI detection, and correctly omits provider for the "no CI" and "unrecognized provider" cases rather than sending the [none]/[unknown] telemetry sentinels, matching the PR description.
  • Single attachment point: confirmed both current call sites (packages/cli/src/rules/plan-check.ts:224, packages/cli/src/rules/recover.ts:177) call reconcileRules, and environment is attached inside reconcileRules itself (packages/cli/src/api/v2.ts:601-604), so neither caller — nor a future one — can send a reconcile body without it.
  • Vendored-schema workaround: verified the local ReconcileRequest intersection type (packages/cli/src/api/v2.ts:573-575) type-checks soundly against the generated reconcile request body type in packages/cli/src/generated/api-v2.d.ts:108-128 — that type has no index signature, and request is passed as a typed variable rather than an object literal, so no excess-property error occurs. The comment explaining why the schema wasn't re-vendored (new error codes would ripple into unrelated typecheck failures) is accurate and the rationale is sound for keeping this as a separate change.
  • replaceAll (adoption-dimensions.ts:174) requires Node ≥15; the package's engines.node is >=22.22.0, so no compatibility concern.
  • Tests are solid: the wire test (api-v2.test.ts) stubs the environment both ways so it doesn't depend on where the suite runs, and the unit tests cover local/off, CI=false beside a provider marker, a recognized provider (including the underscore→hyphen translation), and an unrecognized one.

Minor (non-blocking) observations

  • packages/cli/src/util/adoption-dimensions.ts:171: !ci || provider === NO_CI_PROVIDER || provider === UNKNOWN_DIMENSION — provider === NO_CI_PROVIDER is actually implied by !ci already (per resolveCiProvider's own contract, it returns NO_CI_PROVIDER iff CI is false), so that disjunct is redundant. Harmless and arguably makes the invariant explicit, but worth a second look if it was meant to guard against something else.
  • The new ReconcileRequest type and JSDoc explicitly call out that it's a deletable shim once the schema is re-vendored — nothing to do here, just confirming the "delete me later" marker is clear enough to actually get cleaned up (worth a tracking issue reference if one doesn't already exist, since Refs Report per-case results in verify --json, and send a run environment on v2 reconcile #596 covers the feature but not this follow-up).

No correctness, security, or performance issues found. Nothing else stood out in the diff.

@theCodeDrift

theCodeDrift commented Nov 2, 2026 •

Copy link
Copy Markdown
Member Author

Re: @github-actions — "Claude finished @theCodeDrift's task in 1m 9s"
#597 (comment)

  1. Redundant NO_CI_PROVIDER disjunct: fixed in 384270f. resolveRunEnvironment now returns early off CI and only resolves the provider on CI, which also drops the duplicate CI detection.
  2. Tracking the shim's deletion: not filing a separate issue. The ReconcileRequest intersection is documented in place as a deletable shim, and removing it belongs to the schema re-vendor work, which regenerates the type it intersects.

— AI Coding Agent

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant