Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
49 changes: 20 additions & 29 deletions .github/workflows/compliance-close.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,17 @@ on:
permissions:
contents: read
issues: write
pull-requests: write

jobs:
# Issues only: the PR half of this mechanism (pr-standards.yml's check-compliance
# job, the only thing that ever applied needs:compliance to a pull request) was
# removed as part of dropping the upstream community-PR-governance workflows this
# fork doesn't use (see .github/workflows/pr-standards.yml's removal). The only
# remaining producer of this label, duplicate-issues.yml, labels issues only.
Comment on lines +14 to +18

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

MEDIUM (confidence: high) — unrelated drive-by: 446 lines of CI governance removed in a TUI feature PR.

  # Issues only: the PR half of this mechanism (pr-standards.yml's check-compliance
  # job, the only thing that ever applied needs:compliance to a pull request) was
  # removed as part of dropping the upstream community-PR-governance workflows this
  # fork doesn't use (see .github/workflows/pr-standards.yml's removal).

The PR is titled feat(tui): quota sidebar with Profile and Quota sections and its body describes only the sidebar move. Alongside that it deletes .github/workflows/pr-management.yml (95 lines) and .github/workflows/pr-standards.yml (351 lines) and rewrites this workflow. Nothing in the quota work depends on any of it.

The rationale is plausible — pr-standards.yml reads .github/TEAM_MEMBERS with ref: 'dev', and per .github/claude-review-context.md this fork's default branch is main, not dev, so that getContent call would 404 and the job would error on every community PR. But that makes it a separate, independently reviewable fix, not a rider. Two concrete consequences a reviewer looking at a sidebar diff will not weigh:

  • pr-standards.yml was the only automated enforcement of the conventional-commit PR title format that .github/claude-review-context.md documents as a repo convention (titlePattern = /^(feat|fix|docs|chore|refactor|test)\s*(\([a-zA-Z0-9-]+\))?\s*:/). I grepped the remaining 27 workflows — nothing else checks PR titles. That enforcement is now gone for good.
  • It also carried the needs:description / needs:compliance labelling for community PRs, which is why the edit here is needed at all.

Suggestion: split the three .github/workflows/ changes into their own PR so the removal of contributor-governance automation gets reviewed on its own terms.

Note the change itself reads correctly to me: kind/isPR are removed everywhere they were used, and the surviving producer of needs:compliance (duplicate-issues.yml) does only label issues, as the comment claims.

Comment on lines +14 to +18

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

MEDIUM (high confidence) — Section B: unrelated drive-by change. This CI/governance overhaul has nothing to do with "quota sidebar with Profile and Quota sections" and should be its own PR.

  # Issues only: the PR half of this mechanism (pr-standards.yml's check-compliance
  # job, the only thing that ever applied needs:compliance to a pull request) was
  # removed as part of dropping the upstream community-PR-governance workflows this
  # fork doesn't use (see .github/workflows/pr-standards.yml's removal).

The PR bundles two independent changes:

  1. the TUI feature (packages/cz-cli/src/opencode-plugin/tui-quota*, its tests, the ledger entry);
  2. a CI/governance change — pr-standards.yml deleted (−351), pr-management.yml deleted (−95), this file rewritten, cz-test.yml added (+47), plus --isolate on the test script and a mock-restore afterAll in analytics-agent-session-commands.test.ts.

I'm not arguing the CI change is wrong — the deleted workflows read .github/TEAM_MEMBERS at ref: 'dev', and .github/claude-review-context.md records that dev is not this fork's default branch, so they were almost certainly dead. And cz-test.yml's rationale (turbo's test task never names cz-cli, and blacksmith-* runners were never provisioned here) is a real gap worth closing.

The problem is packaging. Deleting 446 lines of repository automation is the kind of change a reviewer wants to look at on its own terms, and burying it under a feat(tui) title means whoever bisects a CI behavior change later will not think to look here. It also makes this PR unrevertable as a unit: backing out the sidebar takes the new test workflow with it.

Comment on lines +14 to +18

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LOW — repo-governance changes bundled into a TUI feature PR (confidence: high)

  # Issues only: the PR half of this mechanism (pr-standards.yml's check-compliance
  # job, the only thing that ever applied needs:compliance to a pull request) was
  # removed as part of dropping the upstream community-PR-governance workflows this
  # fork doesn't use (see .github/workflows/pr-standards.yml's removal). The only
  # remaining producer of this label, duplicate-issues.yml, labels issues only.

I verified the factual claim: needs:compliance is now produced only by duplicate-issues.yml, which labels issues (gh issue edit, github.event.issue), so narrowing this job to !item.pull_request and dropping pull-requests: write is consistent. The remaining github.rest.issues.update path only needs issues: write. The devmain fix in the CONTRIBUTING link is also correct for this fork.

The note is about packaging, not correctness: deleting pr-standards.yml (351 lines) and pr-management.yml (95 lines) removes automation that gates this repo's own PRs, in the same commit range as a sidebar feature and a credential-provenance refactor. That's the kind of change a reviewer would want to be able to revert independently, and the kind whose blast radius is only visible after the next few PRs land. The PR description does disclose it, so this is a suggestion to split rather than a claim anything is wrong.

One loose end while you're here: close-prs.yml, close-issues.yml, review.yml and triage.yml are still present. If the reason for removing the other two was "upstream community workflows this fork can't run", it'd be worth saying in the PR description whether those four were checked and deliberately kept, so the next person doesn't have to re-derive it.

close-non-compliant:
runs-on: ubuntu-latest
steps:
- name: Close non-compliant issues and PRs after 2 hours
- name: Close non-compliant issues after 2 hours
uses: actions/github-script@f28e40c7f34bde8b3046d885e986cb6290c5673b # v7.1.0
with:
script: |
Expand All @@ -27,18 +31,16 @@ jobs:
per_page: 100,
});

if (items.length === 0) {
core.info('No open issues/PRs with needs:compliance label');
const issues = items.filter((item) => !item.pull_request);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LOW (confidence: high) — with the job now filtering to !item.pull_request and the only remaining write being github.rest.issues.update, the pull-requests: write grant at line 12 is no longer used by anything in this workflow. Dropping it (leaving contents: read + issues: write, which covers listComments / createComment / removeLabel) would keep the token scoped to what the narrowed job actually does.

if (issues.length === 0) {
core.info('No open issues with needs:compliance label');
return;
}

const now = Date.now();
const twoHours = 2 * 60 * 60 * 1000;

for (const item of items) {
const isPR = !!item.pull_request;
const kind = isPR ? 'PR' : 'issue';

for (const item of issues) {
const { data: comments } = await github.rest.issues.listComments({
owner: context.repo.owner,
repo: context.repo.repo,
Expand All @@ -50,13 +52,11 @@ jobs:

const commentAge = now - new Date(complianceComment.created_at).getTime();
if (commentAge < twoHours) {
core.info(`${kind} #${item.number} still within 2-hour window (${Math.round(commentAge / 60000)}m elapsed)`);
core.info(`Issue #${item.number} still within 2-hour window (${Math.round(commentAge / 60000)}m elapsed)`);
continue;
}

const closeMessage = isPR
? 'This pull request has been automatically closed because it was not updated to meet our [contributing guidelines](../blob/dev/CONTRIBUTING.md) within the 2-hour window.\n\nFeel free to open a new pull request that follows our guidelines.'
: 'This issue has been automatically closed because it was not updated to meet our [contributing guidelines](../blob/dev/CONTRIBUTING.md) within the 2-hour window.\n\nFeel free to open a new issue that follows our issue templates.';
const closeMessage = 'This issue has been automatically closed because it was not updated to meet our [contributing guidelines](../blob/main/CONTRIBUTING.md) within the 2-hour window.\n\nFeel free to open a new issue that follows our issue templates.';

await github.rest.issues.createComment({
owner: context.repo.owner,
Expand All @@ -74,22 +74,13 @@ jobs:
});
} catch (e) {}

if (isPR) {
await github.rest.pulls.update({
owner: context.repo.owner,
repo: context.repo.repo,
pull_number: item.number,
state: 'closed',
});
} else {
await github.rest.issues.update({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: item.number,
state: 'closed',
state_reason: 'not_planned',
});
}
await github.rest.issues.update({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: item.number,
state: 'closed',
state_reason: 'not_planned',
});

core.info(`Closed non-compliant ${kind} #${item.number} after 2-hour window`);
core.info(`Closed non-compliant issue #${item.number} after 2-hour window`);
}
77 changes: 77 additions & 0 deletions .github/workflows/cz-test.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
name: cz-test

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

MEDIUM (confidence: high; this is a scope observation, not a defect) — this PR carries four independent changes, and the CI-governance one is a drive-by relative to the stated subject.

The title and description are about moving the quota readout into the sidebar. Alongside that:

  1. the quota/Profile sidebar feature (the actual subject),
  2. deleting pr-standards.yml (351 lines) and pr-management.yml (95 lines), narrowing compliance-close.yml, and adding this workflow,
  3. the connection/env.ts credential-provenance refactor — a security-relevant rewrite of how credentials are selected, which is where the one HIGH finding on this PR lives,
  4. converting four upstream cz_change: comments to banners plus three new ledger entries.

Each is defensible on its own. (2) in particular checks out: I verified duplicate-issues.yml is the only remaining producer of needs:compliance and it labels issues only, so narrowing compliance-close.yml to issues is consistent, and the devmain link fix in the close message is correct for this fork.

The cost is review attention. (3) is the part that most needs careful reading, and it is currently sharing a diff with 446 lines of deleted workflow YAML. Splitting (2) and (4) out as their own PRs — they touch nothing (1) or (3) touch — would let the credential refactor be reviewed on its own terms. Not a blocker if you would rather land it as-is; flagging it because the provenance refactor is the kind of change that benefits most from an isolated diff.

One thing genuinely worth keeping in this PR: the ledger-enforcement step at the bottom of this file. Pointing CI at packages/core/test/model-selection.test.ts and the two clickzetta-* provider tests is what turns UPSTREAM-PATCHES.md's "Verify" lines for entries 7 and 9 into an actual green check. I confirmed all three files exist at those paths.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LOW (scope) — four independent changes in one PR, and the CI-governance one deletes two workflows unrelated to everything else here. (confidence: high)

The PR description is candid that this bundles the quota sidebar, a CI-governance change, the connection-env/provenance refactor, and the upstream-banner conversion. Three of those are genuinely independent of each other, and the credential-provenance refactor in particular is the highest-risk change in the PR (it rewrites how every cz-cli invocation picks an identity) while being the one least connected to the title. Splitting it out would make it reviewable and revertable on its own.

On this workflow specifically, two things worth confirming rather than assuming:

  • pr-standards.yml had two jobs, check-standards and check-compliance. The compliance-close.yml comment accounts for check-compliance, but not for whatever check-standards enforced. If any of its checks are configured as required status checks on main, deleting the workflow leaves PRs waiting on a check that can never report. Worth verifying the branch-protection settings before merge.
  • .github/claude-review-context.md documents a conventional-commit convention for PR and commit titles. If check-standards was what enforced that, it is now unenforced. That may be fine for a fork, but it is a policy change rather than a cleanup.

cz-test.yml itself looks right: pull_request (not pull_request_target), permissions: contents: read, pinned action SHAs, GitHub-hosted runners, and the extra step running the two cz-owned suites inside upstream packages so the ledger's Verify lines for entries 7 and 9 are actually backed by a check. I cannot run it, so I have not confirmed the steps pass.


# Runs cz-cli's OWN unit tests and typecheck. Distinct from `test.yml`, which
# only covers upstream opencode packages (turbo.json's `test` task names
# opencode/@opencode-ai/{core,app,ui,session-ui} exclusively — cz-cli,
# clickzetta-sdk and clickzetta-ai-gateway are not in that list and are never
# exercised there) and which is stuck queued forever on this fork because
# `blacksmith-*` runners were never provisioned for clickzetta/cz-cli. This
# workflow uses GitHub-hosted runners so it actually executes.
Comment on lines +3 to +9

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

MEDIUM — scope: this PR bundles at least three unrelated changes under a title and description about the quota sidebar. Confidence: high.

The description is one sentence — "Moves the balance/quota readout into the session sidebar and adds a Profile section showing the active connection's identity." What the diff actually contains:

  1. The quota sidebar featuretui-quota*, UPSTREAM-PATCHES.md hook entry 5. Matches the title.
  2. CI governance rework — this new workflow, plus deleting pr-standards.yml (351 lines) and pr-management.yml (95 lines), plus rewriting compliance-close.yml to drop its PR half. Removing the two pull_request_target workflows is a defensible call for a fork, and the reasoning left in compliance-close.yml:15-19 is good, but none of it is mentioned anywhere a reviewer or a future bisect would look.
  3. A connection-env / credential-provenance refactor — new connection/env.ts, rewritten pickCredential, rewritten profile-env.ts, run-cli.ts, workspace.ts, three new test files. This is the largest and riskiest part of the diff (see my comments on run-cli.ts:561 and config.ts:130) and it is invisible from the title.
  4. Upstream-patch banner conversions (cz_change: → wrapped banners) plus ledger entries 7/8/9. This one is genuinely coupled to nothing else and reads as pure compliance work.

Items 2 and 3 each want their own PR, or at minimum their own paragraph in the description. #3 in particular changes auth precedence — that is not something to land under a "move a readout into the sidebar" heading, and CONTRIBUTING.md's own checklist asks contributors to confirm they have not included unrelated changes.

On the workflow itself: the rationale comment is clear and correct about turbo.json's test task not covering cz-cli, and ./.github/actions/setup-bun does run bun install, so the missing explicit install step is fine.


on:
push:
branches: [main]
pull_request:
workflow_dispatch:

concurrency:
group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}
cancel-in-progress: true

permissions:
contents: read

jobs:
test:
runs-on: ubuntu-latest
steps:
- name: Checkout repository
uses: actions/checkout@34e114876b0b11c390a56381ad16ebd13914f8d5 # v4.3.1

- name: Setup Bun
uses: ./.github/actions/setup-bun

- name: Typecheck cz-cli
working-directory: packages/cz-cli
run: bun run typecheck

- name: Run cz-cli unit tests
working-directory: packages/cz-cli
# test script runs with --isolate: several suites (e.g.
# analytics-agent-session-commands.test.ts) mock.module() their own src,
# which bun's default shared-process test run leaks across files sharing
# that process — a later network-boundary suite (task-condition-flow,
# task-merge) that expects the REAL module then reads this file's fixture
# data instead. --isolate gives each file its own global object so a
# leaked mock cannot reach another file at all.
#
# analytics-agent-session-commands.test.ts ALSO restores its mocks in its
# own afterAll (see the comment there). That is not redundant with
# --isolate: it fixes the one suite that leaked, while --isolate is the
# unconditional guarantee for every suite that has not been fixed the same
# way, or will not remember to be. Keep both — the per-file fix is cheap
# insurance against exactly this suite regressing if --isolate is ever
# removed for its cost (a fresh global per file is slower, and it can also
# hide a REAL cross-file coupling that would still bite in the shipped
# binary, where every test file's module cache is shared).
#
# Intent, confirmed: --isolate is the PRIMARY guarantee for the whole
# suite (every file, not just the one known leaker), accepted as a
# change to the shared local/CI test contract, not a CI-only knob. The
# afterAll restore and the two test-only resets added alongside it
# (clearUnservedHostForTest, clearUserNameCacheForTest in
# tui-quota-data.ts) are deliberately kept as belt-and-suspenders on top
# of it, per the reasoning above and in each of those call sites.
run: bun run test
Comment on lines +38 to +65

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

MEDIUM (confidence: high) — this workflow runs only packages/cz-cli, so the packages/core test this PR adds still never executes in CI.

packages/core/test/model-selection.test.ts is new here, and UPSTREAM-PATCHES.md entry 9 names it as the enforcement for the extracted model-selection chain ("goes RED if resolveModelSelection's tier order or fallback-on-dead-ref behavior regresses"). The only workflow that would run it is test.yml via @opencode-ai/core#test (turbo.json:16) — which this PR's own description says is stuck queued forever on unprovisioned blacksmith-* runners. Same for the two tests entry 7 names, packages/opencode/test/provider/clickzetta-*.test.ts.

So the ledger's re-baseline checklist points at three test files that no green check ever covers on this fork. Worth adding a step here for the cz-owned tests living inside upstream packages, e.g.:

      - name: Run cz-owned tests inside upstream packages
        run: |
          bun test packages/core/test/model-selection.test.ts \
                   packages/opencode/test/provider/clickzetta-discovery.test.ts \
                   packages/opencode/test/provider/clickzetta-context-limit.test.ts

Flagging as a question rather than a defect if the intent was to scope this workflow strictly to packages/cz-cli and fix test.yml's runners separately — but in that case entry 9's Verify line overstates what CI actually guarantees today.


# UPSTREAM-PATCHES.md's re-baseline checklist points at these three files
# as the enforcement for their respective ledger entries (7, 9), but they
# live inside upstream packages and are only named by test.yml's
# @opencode-ai/core#test / opencode#test turbo tasks — the workflow stuck
# queued forever on unprovisioned blacksmith-* runners. Run them directly
# here so the ledger's claim is actually backed by a green check on this
# fork, without pulling every upstream package's suite into this workflow.
- name: Run cz-owned tests inside upstream packages
run: |
(cd packages/core && bun test test/model-selection.test.ts)
(cd packages/opencode && bun test test/provider/clickzetta-discovery.test.ts test/provider/clickzetta-context-limit.test.ts)
95 changes: 0 additions & 95 deletions .github/workflows/pr-management.yml

This file was deleted.

Loading
Loading