fix(breeze): support gh auth status without json - #309
Conversation
bingran-you
left a comment
There was a problem hiding this comment.
LGTM. The approach is clean: drop the non-existent --json flag, parse the human-readable output from gh auth status --active --hostname <host>, and keep handling both quoted ('repo', 'workflow') and unquoted comma-separated scope styles. Nice touches:
- The new regression test asserts
--json hostsis no longer in argv — that pins down #308's root cause. parseScopesfilters(none)/none, so a zero-scope token won't masquerade as scope"(none)".- The login parser uses
Logged in to <host> accountas the anchor, which avoids false positives from the summary/keyring-status lines.
Sanity-checked against real gh 2.86.0 output locally — it writes to stdout (matches what runChecked returns), so the parser sees what we expect. Verified the smoke command in the PR body (breeze doctor --allow-repo …) is the right end-to-end regression check for #308.
PR is behind main but mergeable — just needs a rebase before merging. The failing tree-sync check is an unrelated auth/TREE_REPO_TOKEN issue, not caused by this change.
This reply was drafted by breeze, an autonomous agent running on behalf of the account owner.
yuezengwu
left a comment
There was a problem hiding this comment.
No findings. The change removes the gh auth status --json hosts dependency, parses the stable text output from gh auth status --active --hostname <host>, and preserves the needed login/protocol/scope behavior.
Verified locally:
pnpm exec vitest run tests/breeze/breeze-daemon-identity.test.tspnpm typecheckpnpm buildBREEZE_DIR=$(mktemp -d) node dist/cli.js breeze doctor --allow-repo agent-team-foundation/first-tree
The remaining failing tree-sync check fails while cloning first-tree-context due to invalid tree repo credentials, before this PR's code runs, so I do not consider it caused by this change.
This reply was drafted by breeze, an autonomous agent running on behalf of the account owner.
yuezengwu
left a comment
There was a problem hiding this comment.
No findings from my review. I verified the parser against local gh auth status --active --hostname github.com output from gh 2.91.0, and ran:\n\n- pnpm exec vitest run tests/breeze/breeze-daemon-identity.test.ts\n- pnpm typecheck\n\nBoth passed locally. The PR also keeps the daemon resolver off gh auth status --json hosts, which addresses #308 directly. The remaining failing tree-sync check appears unrelated to this patch.\n\nThis reply was drafted by breeze, an autonomous agent running on behalf of the account owner.
Dismissing duplicate approval; an earlier yuezengwu review already covered this.
serenakeyitan
left a comment
There was a problem hiding this comment.
LGTM. Text-parsing gh auth status --active --hostname <host> is the right call — the --json hosts flag isn't uniformly supported across gh releases, and the new parser correctly handles the shell-quoted scopes + (none) sentinel + Git-protocol line. Regression test pins the no---json contract. Nice fix.
5fe69b5 to
3eb734b
Compare
|
Rebased this PR onto current
All four passed locally on the rebased head. GitHub now shows
So the patch itself is validated and approved, but the PR is still blocked on the repo's unrelated This reply was drafted by breeze, an autonomous agent running on behalf of the account owner. |
|
Please proceed with this approach — confirmed via end-to-end smoke test. |
|
ok |
|
Acknowledged — but flagging before I merge: this PR is now 50 commits behind main and has merge conflicts (mergeStateStatus: DIRTY). Notably, 'feat: port breeze to @first-tree/auto package' (#345) has since restructured the breeze code path this fix touches, so the original patch likely needs to be re-applied (or may already be obsolete) against the new layout rather than rebased mechanically. Could you confirm: (1) rebase + manually resolve conflicts against the new @first-tree/auto layout then merge, (2) close this PR if the underlying #308 issue is already addressed by the breeze restructure, or (3) open a fresh PR re-doing the fix on top of current main. Holding off on the merge until you confirm. This reply was drafted by github-scan, an autonomous agent running on behalf of the account owner. |
|
Surfacing this for human review: a new comment landed on this PR. Could you summarize what's outstanding here — is the |
|
Thanks for the ping — could you clarify what's outstanding here? Specifically: (1) is the latest comment requesting a code change, or just asking a question? (2) has this been tested against an older |
|
test |
2 similar comments
|
test |
|
test |
|
Triage ping: this PR surfaced for human review based on a recent comment. Could you summarize what's still blocking merge — is there a question for reviewers, a failing check to address, or is this ready for another look? A one-line status will help route it. |
Fixes #308.
Summary
gh auth status --json hostsdependency with plaingh auth status --active --hostname <host>parsinggh auth status--jsonTesting
pnpm exec vitest run tests/breeze/breeze-daemon-identity.test.tspnpm typecheckpnpm buildBREEZE_DIR=$(mktemp -d)thennode dist/cli.js breeze doctor --allow-repo agent-team-foundation/first-tree