-
Notifications
You must be signed in to change notification settings - Fork 7
feat(tui): quota sidebar with Profile and Quota sections #76
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.
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
Changes from all commits
da6aa2f
326d636
b40281f
4ae3bef
d7ef2c5
7b18cc7
56f5221
dfca7ba
78ae5ad
0fb91cc
7a1ddf3
c1019f7
70ca0b3
e417ea5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
I'm not arguing the CI change is wrong — the deleted workflows read 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
Comment on lines
+14
to
+18
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: The note is about packaging, not correctness: deleting One loose end while you're here: |
||
| 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: | | ||
|
|
@@ -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); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LOW (confidence: high) — with the job now filtering to |
||
| 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, | ||
|
|
@@ -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, | ||
|
|
@@ -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`); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| name: cz-test | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
Each is defensible on its own. (2) in particular checks out: I verified 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 On this workflow specifically, two things worth confirming rather than assuming:
|
||
|
|
||
| # 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
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 On the workflow itself: the rationale comment is clear and correct about |
||
|
|
||
| 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MEDIUM (confidence: high) — this workflow runs only
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.tsFlagging as a question rather than a defect if the intent was to scope this workflow strictly to |
||
|
|
||
| # 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) | ||
This file was deleted.
There was a problem hiding this comment.
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.
The PR is titled
feat(tui): quota sidebar with Profile and Quota sectionsand 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.ymlreads.github/TEAM_MEMBERSwithref: 'dev', and per.github/claude-review-context.mdthis fork's default branch ismain, notdev, so thatgetContentcall 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.ymlwas the only automated enforcement of the conventional-commit PR title format that.github/claude-review-context.mddocuments 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.needs:description/needs:compliancelabelling 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/isPRare removed everywhere they were used, and the surviving producer ofneeds:compliance(duplicate-issues.yml) does only label issues, as the comment claims.