Repository navigation
refactor(agents): port GitHub quota and identity handling to Go #760
Description
Problem
dt_gh and the login and repository readers in packages/agents/source/lib/lib.sh run every GitHub read the unattended workflows make, and PR writes depend on them for the account check. The rate-limit classifier and the identity check are shell, and a mistake either hides an authentication failure or turns a real quota error into a mismatch.
Part of #588.
Acceptance criteria
- AC1: Given a
ghcall that fails with an authentication or permission error, when it is classified, then the original status and text reach the caller unchanged and it is never reported as a quota error or an identity mismatch. - AC2: Given a failed
ghcall, when its standard error is read, then only a line starting withgh:orGraphQL:followed by a space and containing a documented primary or secondary quota message classifies as a rate limit, which yields status 76 followed by the original error unchanged. Matching text on standard output or on an unprefixed line does not classify. - AC3: Given the
dt_ghrunner, when a call succeeds or fails, then standard output and standard error pass through unchanged and the original status is kept; a rate-limit failure prints guidance that distinguishes REST, GraphQL and secondary limits, and the runner never retries and never looks up the budget. - AC4: Given the active login and repository readers run against stubs for success, a failed call and an empty answer, then status, standard output and standard error equal the shell reader's for each.
- AC5: Given the consuming
create-pridentity check, whenghfails with an authentication or permission error, then the status and diagnostic are kept and no mismatch message appears; given a wrong successful login, then the existing mismatch refusal prints. - AC6: Given a consumer on an older pinned binary that lacks the replacement, when it runs after this change, then it sees today's behaviour. Given a replacement that is present and fails or returns partial output, then the caller refuses with the replacement's status and diagnostic and never runs the shell path.
- AC7: Generated skill output (the copies of
lib.shunder the generated skill directories) is regenerated from the canonical source in the same pull request, andmise run check-generated-skillspasses.
Scope and non-goals
Port the GitHub subprocess contract: running gh, classifying quota errors from standard error, and reading the active login and repository. Keep status 76 for a rate limit and keep the original error text.
Non-goal: changing any behaviour a consumer on an older pinned binary relies on. Non-goal: removing the shell fallback, which is deferred to a follow-up issue filed under D9 when this lands.
Approach
Meet the acceptance criteria with failing cases first. The mechanism is the implementer's choice within those outcomes.
Dependencies
Blocked by #753, because this is enforcement. Part of #588. The fallback removal is deferred to a follow-up issue filed when this lands. Touches a gate path.
Size
S. One pull request.
Verify plan
- AC1: stubbed
ghreturning authentication and permission failures; assert unchanged status and text and no classification. - AC2: a table of standard-error lines (documented messages, prefixed and unprefixed, on stdout and stderr); assert status 76 and the original error only for the prefixed quota lines.
- AC3: stubs for success, failure and each quota kind; assert stdout and stderr passthrough, the REST, GraphQL and secondary guidance text, no second call and no budget request.
- AC4: the login and repository cases at
tests/skills/source/lib/lib.batsrun against both paths, plus rows for the empty answer; compare status, standard output and standard error. - AC5: run the
create-pridentity check with stubs for an authentication failure, a permission failure and a wrong login; assert the kept status and diagnostic, no mismatch text for the first two, and the mismatch refusal for the third. - AC6: stub binaries for absent, present-and-failing and present-with-partial-output; assert the legacy result, then the replacement's status and diagnostic, with zero fallback and zero repeat execution.
- AC7:
mise run generate-skillsleaves no diff andmise run check-generated-skillspasses. - Definition of done: D1 each AC is verified at the merged head, with the commands and results cited in the pull request's Verification. D2 an independent review approves the exact merged head, and every finding has a recorded disposition. D3 required checks pass with no gate loosened. D4 the leak scans the repository specifies are clean, and every post made for this work (comments, descriptions) passes the per-post leak review before it is posted. D5 every commit is signed. D6 the documentation matches, or the description says
No doc impactwith the reason. D7 every commit carries anAssisted-bytrailer and the pull request carries theprovenance:agent-assistedlabel. D8 the change stays within the pull request size limits. D9 follow-up work, including deferred review findings, is filed as issues, each with a Problem section and numbered acceptance criteria and linking this issue; the pull request's Related section lists them, and any code that defers work links its issue. D10 the merge closes this issue through a closing reference, the project board shows the state Done, and a finalStatus:comment names the merge commit.
Risks
Hiding an authentication failure as a quota error would send callers to wait instead of fixing credentials. AC1 and AC2 pin both. Undo: revert the change.
Operator authority
A standing rule names the operator: the canonical packages/agents/source/lib/lib.sh and the generated scripts/lib.sh copies are golden, constitutional or owner paths (.agents/agent-skills.conf:4,5), so the pull request needs the owner's exact-head review at merge.
Activity
tbhb-agent commentedon Oct 26, 2026
Refinement: NOT READY — behavior parity, verification, and owner authority need clarification
Model: gpt-6.1-sol
Checked
Read live #760 and its comments (none), plus named issues #588 and #753 and their comments. Verified the independent clone’s origin identifies
tbhb-dev/devtools. Repository evidence refers toorigin/mainatf130498bd7383fe02f8b9d21965f555866abd98c.packages/agents/source/lib/lib.sh:823,packages/agents/source/lib/lib.sh:866,packages/agents/source/lib/lib.sh:875).Gaps
High — Behavior parity is only partly expressed as observable requirements. AC3 delegates the contract to an existing test suite rather than stating its outcomes. Preserve successful stdout/stderr and status; restrict classification to failed calls and qualifying stderr lines; retain quota-specific diagnostics and original errors; make no retry or budget request; and reject empty/null viewer identities. Existing evidence includes
tests/skills/source/lib/lib.bats:630,tests/skills/source/lib/lib.bats:685,tests/skills/source/lib/lib.bats:701,tests/skills/source/lib/lib.bats:710, andtests/skills/source/lib/lib.bats:734. State these outcomes explicitly without prescribing test organization.High — Verify does not prove every AC or the compatibility boundary. “Write its failing case first” does not identify expected results. Authentication fixtures cannot prove generated output or the full quota matrix.
Map AC1 to authentication and permission statuses, preserved diagnostics, and absence of a false mismatch at a consuming identity check; mismatch handling lives outside the reader (
packages/agents/source/skills/pr/shared/scripts/create-pr.sh:143). Map AC2 to qualifying messages, negative lookalikes, stdout-only text, and successful calls. Map AC3 to the relevant behavior cases exercising the Go path. Map AC4 to generation consistency, such asmise run check-generated-skills(AGENTS.md:203). Include checks for unsupported-command compatibility and supported-command failure/partial-output refusal.High — Operator authority is incorrect. “None” conflicts with the protected source and generated library paths. The source is covered by golden, constitutional, and owner-path patterns (
.agents/agent-skills.conf:4,.agents/agent-skills.conf:5,.agents/agent-skills.conf:9). Name the existing owner-review requirements and exact-head approval process, consistent with Replace complex shell with tested Go, starting with gates and merging #588 andAGENTS.md:68. Retain build(agents): protect replacement Go enforcement paths #753 as the prerequisite protecting replacement enforcement before caller activation. This grants no implementation or write authority.Medium — The body prescribes design outside Operator authority. Approach mandates an
agentreposubcommand and a shell probing procedure. Move that command structure and caller mechanism into the eventual PR. Keep the observable compatibility requirements: older supported deployments retain existing behavior; a recognized replacement failure refuses without fallback. Move AC3’s named test-file prescription into Verify.Medium — Completion requirements lack complete planning coverage. Neither refactor(agents): port GitHub quota and identity handling to Go #760 nor parent Replace complex shell with tested Go, starting with gates and merging #588 supplies the full D1–D10 contract below. Add explicit coverage for merged-head evidence, review and finding disposition, required checks, named leak scans, signing, documentation, provenance, size limits, follow-ups, and closure reporting. build(agents): protect replacement Go enforcement paths #753’s completion paragraph concerns its own protection change.
Low — The deferred fallback-removal issue is unnamed. Non-goals says “a separate XS issue owns” removal but supplies no issue number. Name that follow-up and classify it as deferred work rather than a blocker, or remove the assertion that an existing issue owns it.
Split
S, one PR is reasonable for this subprocess contract, focused regression coverage, and generated output. No further implementation split is needed. Fallback removal remains a separate follow-up.
Definition of done
The definition of done for #760 (Feature). Tick each item and give its evidence.
Boxes indicate planning coverage, not completed work. No item is fully covered by the reviewed requirements.
D1 The author verified each AC on the merged head, and the Verification section of the pull request cites each AC ID with its evidence: the command and its result, or a link.
Evidence: Verify names AC IDs but omits merged-head verification and PR evidence requirements.
D2 An independent reviewer approved the exact merged head, and the author resolved each finding or filed it as a follow-up.
Evidence: No complete independent-review and finding-disposition requirement appears.
D3 The required checks pass on that head, and any loosened gate has the owner's approval of that exact head.
Evidence: Replace complex shell with tested Go, starting with gates and merging #588 AC8 requires checks and owner protections, but does not state this complete completion condition.
D4 The leak scans come back clean: gitleaks, devtools'
check-leaks.shwith the repository's private-strings list, and the leak review of each post.Evidence: The named scans and post-review requirement are absent.
D5 The author signed each commit, the squash included.
Evidence: Replace complex shell with tested Go, starting with gates and merging #588 preserves signing checks as migrated behavior; it does not require signing every commit for this feature.
D6 The docs match the change (README,
docs/, CHANGELOG or help text), or the Verification section says "No doc impact" and why.Evidence: Neither documentation verification nor its stated alternative appears.
D7 The commits and the squash have Assisted-by trailers, and the pull request has the
provenance:agent-assistedlabel.Evidence: Neither provenance completion requirement appears.
D8 The pull request stays within the size limits, or the owner approved its exact head over the hard limit.
Evidence: “S” estimates scope but does not require size-limit compliance or the exact-head exception.
D9 The author files follow-ups and deferred findings as issues with at least a Problem and numbered AC and lists them under the Related section of the pull request. Any work left in the code links its issue.
Evidence: The unnamed fallback-removal follow-up does not establish this handling contract.
D10 The merge closes the issue through a closing reference and the board shows Done. The final
Status:comment names the merge commit.Evidence: Closing-reference, board-state, and final-status requirements are absent.
Round: 1 of 3
tbhb-agent commentedon Oct 26, 2026
Refinement: NOT READY — fallback ownership and verification coverage remain incomplete
Model: gpt-6.1-sol
Checked
Read live #760 and its comment once, plus the named issues #588, #753 and #772. Verified the independent clone’s origin identifies
tbhb-dev/devtools. Source references below are againstorigin/mainatf130498bd7383fe02f8b9d21965f555866abd98c..agents/agent-skills.conf:4,.agents/agent-skills.conf:5and.agents/agent-skills.conf:9.Gaps
Medium — refactor(agents): remove the shell fallback for discovery and worktree readiness #772 does not own this fallback removal. refactor(agents): remove the shell fallback for discovery and worktree readiness #772 covers discovery and worktree readiness from refactor(agents): port skill capability and worktree context discovery to Go #755, and explicitly excludes “removing a fallback owned by another port.” Correct the Non-goals and Dependencies statements: identify an issue that actually owns GitHub quota/identity fallback removal, or describe that removal as deferred without claiming refactor(agents): remove the shell fallback for discovery and worktree readiness #772 owns it. It remains deferred work, not a blocker.
Medium — Observable parity is incomplete for the subprocess wrapper. AC3 requires equality for the login and repository readers, but the scope also ports
dt_gh. Its current contract includes successful stdout/stderr passthrough, distinct REST/GraphQL/secondary quota guidance, and no automatic retry or budget lookup (packages/agents/source/lib/lib.sh:823,packages/agents/source/lib/lib.sh:833,packages/agents/source/lib/lib.sh:859). The Verify plan references existing cases covering these behaviors, but the requirements do not explicitly preserve them for the wrapper. Add observable parity outcomes for these streams, diagnostics and side effects, then map them to checks. Existing evidence includestests/skills/source/lib/lib.bats:630,tests/skills/source/lib/lib.bats:666,tests/skills/source/lib/lib.bats:710andtests/skills/source/lib/lib.bats:746.Medium — Reader-only checks cannot prove the identity-mismatch requirements. AC1 forbids reporting authentication/permission failures as identity mismatches, and AC3 names a mismatch case. The mismatch comparison occurs outside the readers: the consuming check first propagates a reader failure, then compares a successful login (
packages/agents/source/skills/pr/shared/scripts/create-pr.sh:143). Add verification through a consuming identity check: authentication/permission failure preserves its status and diagnostic without a mismatch message; a successful wrong login produces the existing mismatch refusal. Keep these as behavior requirements without prescribing test organization.Low — The completion paragraph leaves several checklist conditions unspecified. Complete D6’s
No doc impactalternative with its reason and location in Verification; name D7’sAssisted-bytrailers andprovenance:agent-assistedlabel; include D9’s issue content, Related links and code references; and include D10’s closing reference and board Done state. The checklist below identifies the remaining coverage precisely.Split
One S-sized PR remains reasonable for this contract, focused regression coverage and generated output. No further split is needed. Fallback removal stays separate.
Definition of done
The definition of done for #760 (Feature). Tick each item and give its evidence.
Ticks indicate planning coverage, not completed work.
D1 The author verified each AC on the merged head, and the Verification section of the pull request cites each AC ID with its evidence: the command and its result, or a link.
Evidence: Verify’s D1 explicitly requires merged-head verification and commands/results cited in the pull request’s Verification.
D2 An independent reviewer approved the exact merged head, and the author resolved each finding or filed it as a follow-up.
Evidence: Verify’s D2 requires independent approval of the exact merged head and a recorded disposition for every finding; D9 requires follow-up work to be filed as issues.
D3 The required checks pass on that head, and any loosened gate has the owner's approval of that exact head.
Evidence: The completion paragraph requires checks to pass with no gate loosened, within its merged-head verification requirement.
D4 The leak scans come back clean: gitleaks, devtools'
check-leaks.shwith the repository's private-strings list, and the leak review of each post.Evidence: Verify’s D4 requires the repository-specified leak scans to be clean.
D5 The author signed each commit, the squash included.
Evidence: Verify’s D5 requires every commit to be signed, which includes the squash commit.
D6 The docs match the change (README,
docs/, CHANGELOG or help text), or the Verification section says "No doc impact" and why.Evidence: D6 allows
No doc impactin the description but omits the explanation and its placement in Verification.D7 The commits and the squash have Assisted-by trailers, and the pull request has the
provenance:agent-assistedlabel.Evidence: D7 says “provenance trailers and the provenance label” without identifying the required trailer, label or squash coverage.
D8 The pull request stays within the size limits, or the owner approved its exact head over the hard limit.
Evidence: Verify’s D8 requires compliance with the pull request size limits; no exception is requested.
D9 The author files follow-ups and deferred findings as issues with at least a Problem and numbered AC and lists them under the Related section of the pull request. Any work left in the code links its issue.
Evidence: D9 requires follow-up issues but omits their minimum content, Related listing and code links.
D10 The merge closes the issue through a closing reference and the board shows Done. The final
Status:comment names the merge commit.Evidence: D10 requires closure and the final merge-commit comment, but omits the closing reference and board Done state.
Round: 2 of 3
tbhb-agent commentedon Oct 26, 2026
Refinement: READY — bounded parity contract with adequate verification and owner authority
Model: gpt-6.1-sol
Checked
Read live #760 and its two comments once, plus named issues #588 and #753. Verified the independent clone’s origin identifies
tbhb-dev/devtools. Repository evidence is againstorigin/mainatf130498bd7383fe02f8b9d21965f555866abd98c.packages/agents/source/lib/lib.sh:830,packages/agents/source/lib/lib.sh:875,packages/agents/source/lib/lib.sh:886andpackages/agents/source/skills/pr/shared/scripts/create-pr.sh:135. AC2 supplies the quota-status exception to AC3’s general status preservation.tests/skills/source/lib/lib.bats:630,tests/skills/source/lib/lib.bats:666,tests/skills/source/lib/lib.bats:685,tests/skills/source/lib/lib.bats:710andtests/skills/source/lib/lib.bats:734..agents/agent-skills.conf:4,.agents/agent-skills.conf:5and.agents/agent-skills.conf:9. The body correctly names owner review of the exact head.Gaps
None.
Split
One S-sized PR is reasonable for this subprocess contract, focused regression coverage and generated output. Fallback removal remains separate deferred work.
Definition of done
The definition of done for #760 (Feature). Tick each item and give its evidence.
Ticks indicate planning coverage, not completed work.
D1 The author verified each AC on the merged head, and the Verification section of the pull request cites each AC ID with its evidence: the command and its result, or a link.
Evidence: Verify’s D1 explicitly requires merged-head verification and commands/results cited in the pull request’s Verification.
D2 An independent reviewer approved the exact merged head, and the author resolved each finding or filed it as a follow-up.
Evidence: D2 requires independent approval of the exact merged head and disposition of every finding; D9 covers follow-up issues.
D3 The required checks pass on that head, and any loosened gate has the owner's approval of that exact head.
Evidence: The completion contract requires passing checks and prohibits gate loosening.
D4 The leak scans come back clean: gitleaks, devtools'
check-leaks.shwith the repository's private-strings list, and the leak review of each post.Evidence: D4 requires repository-specified scans and per-post leak review. The scan tasks are defined at
.devtools/tasks/security.toml:24and.devtools/tasks/leaks.toml:2.D5 The author signed each commit, the squash included.
Evidence: D5 requires every commit to be signed, encompassing the squash.
D6 The docs match the change (README,
docs/, CHANGELOG or help text), or the Verification section says "No doc impact" and why.Evidence: D6 requires matching documentation or a reasoned
No doc impactstatement in the PR description’s verification contract.D7 The commits and the squash have Assisted-by trailers, and the pull request has the
provenance:agent-assistedlabel.Evidence: D7 explicitly requires the trailer on every commit and the named provenance label.
D8 The pull request stays within the size limits, or the owner approved its exact head over the hard limit.
Evidence: D8 requires compliance with PR size limits; no exception is requested.
D9 The author files follow-ups and deferred findings as issues with at least a Problem and numbered AC and lists them under the Related section of the pull request. Any work left in the code links its issue.
Evidence: D9 explicitly covers deferred findings, minimum issue content, Related entries and code links. Non-goals also assigns fallback-removal tracking to this requirement.
D10 The merge closes the issue through a closing reference and the board shows Done. The final
Status:comment names the merge commit.Evidence: D10 explicitly requires the closing reference, board Done state and final merge-commit comment.
Round: 3 of 3