Implementation Plan: token_summary_appender — Replace gh pr edit/view with REST API#551
Conversation
…appender Bypasses the broken GraphQL path that fatal-errors on repos with Projects Classic history. Adds _parse_pr_url_parts() helper, replaces subprocess calls with gh api REST equivalents, and updates all test mocks accordingly. REQ-TEST-001: guard test added to assert gh pr edit/view are absent from source.
Trecek
left a comment
There was a problem hiding this comment.
AutoSkillit PR Review — Verdict: changes_requested
|
|
||
| REQ-TEST-001: verifies both read and write operations use gh api (REST). | ||
| """ | ||
| from autoskillit.core.paths import pkg_root |
There was a problem hiding this comment.
[warning] slop: Redundant local import inside function body. pkg_root is already imported at module level (used in test_tsa1_token_summary_appender_script_exists at line 105). Remove the from autoskillit.core.paths import pkg_root local import.
There was a problem hiding this comment.
Investigated — this is intentional. Module-level imports (lines 7-15) contain no 'from autoskillit.core.paths import pkg_root'. The import at line 103 (inside test_tsa1_token_summary_appender_script_exists) is itself a local function-body import, not a module-level import. Same pattern at line 132. The local import at line 113 is consistent with the file-wide convention of lazy per-function imports, not a redundancy.
Trecek
left a comment
There was a problem hiding this comment.
AutoSkillit review found 2 blocking issues. See inline comments.
Summary
token_summary_appender.pycallsgh pr viewandgh pr edit --body, both of which invokethe
updatePullRequestGraphQL mutation internally. GitHub now returns a fatal error on reposthat ever had Projects Classic enabled:
The fix adds
_parse_pr_url_parts(pr_url)to extract(owner, repo, number)from the PR URL, then replacesgh pr viewwithgh api repos/{owner}/{repo}/pulls/{number} --jq '.body'andgh pr edit --bodywithgh api repos/{owner}/{repo}/pulls/{number} --method PATCH --field body=<new_body>. All test mocks are updated to match the new REST API call signatures, and a source-level guard test verifies that the broken GraphQL-backed commands are absent.Requirements
GH (GitHub API Migration)
gh api repos/TalonT-Org/AutoSkillit/pulls/{number} --jq '.body') instead ofgh pr view.gh api repos/TalonT-Org/AutoSkillit/pulls/{number} --method PATCH --field body=...) instead ofgh pr edit --body._extract_pr_url().TEST (Verification)
gh pr editorgh pr viewsubprocess calls remain).test_token_summary_appender.pytests must be updated to mock the REST API calls instead ofgh pr edit/gh pr view.Architecture Impact
Process Flow Diagram
%%{init: {'flowchart': {'nodeSpacing': 40, 'rankSpacing': 50, 'curve': 'basis'}}}%% flowchart TB %% CLASS DEFINITIONS %% classDef terminal fill:#1a237e,stroke:#7986cb,stroke-width:2px,color:#fff; classDef stateNode fill:#004d40,stroke:#4db6ac,stroke-width:2px,color:#fff; classDef handler fill:#e65100,stroke:#ffb74d,stroke-width:2px,color:#fff; classDef phase fill:#6a1b9a,stroke:#ba68c8,stroke-width:2px,color:#fff; classDef detector fill:#b71c1c,stroke:#ef5350,stroke-width:2px,color:#fff; classDef newComponent fill:#2e7d32,stroke:#81c784,stroke-width:2px,color:#fff; START([PostToolUse fires]) subgraph Ingestion ["Event Ingestion"] direction TB ParseEvent["● main()<br/>━━━━━━━━━━<br/>stdin → tool_name, tool_response_raw"] ExtractURL["_extract_pr_url()<br/>━━━━━━━━━━<br/>regex search for github.com PR URL"] NoURL{"PR URL<br/>found?"} ParseParts["● _parse_pr_url_parts()<br/>━━━━━━━━━━<br/>extract owner, repo, number<br/>from PR URL"] NoParts{"parts<br/>valid?"} end subgraph SessionLoad ["Session Aggregation"] direction TB LoadSessions["_load_sessions()<br/>━━━━━━━━━━<br/>filter sessions.jsonl by pipeline_id<br/>aggregate token_usage.json per step"] NoSessions{"sessions<br/>found?"} end subgraph GitHubRead ["● GitHub REST Read"] direction TB APIRead["● gh api repos/{owner}/{repo}/pulls/{number}<br/>━━━━━━━━━━<br/>--jq '.body'<br/>(was: gh pr view --json body --jq .body)"] ReadFail{"returncode<br/>== 0?"} AlreadyHas{"body has<br/>## Token Usage<br/>Summary?"} EmptyBody{"body<br/>empty?"} end subgraph TableBuild ["Table Construction"] FormatTable["_format_table()<br/>━━━━━━━━━━<br/>build markdown token table<br/>from aggregated step data"] Concat["concat body + table<br/>━━━━━━━━━━<br/>new_body = current_body + table"] end subgraph GitHubWrite ["● GitHub REST Write"] direction TB APIPatch["● gh api repos/{owner}/{repo}/pulls/{number}<br/>━━━━━━━━━━<br/>--method PATCH --field body=new_body<br/>(was: gh pr edit --body)"] WriteFail{"CalledProcess<br/>Error?"} end EXIT0([EXIT 0]) EXIT0_DIAG([EXIT 0 + stderr diagnostic]) EXIT1([EXIT 1]) COMPLETE([EXIT 0 — summary appended]) START --> ParseEvent ParseEvent --> ExtractURL ExtractURL --> NoURL NoURL -->|"no URL"| EXIT0 NoURL -->|"URL found"| ParseParts ParseParts --> NoParts NoParts -->|"parse failed"| EXIT0 NoParts -->|"owner/repo/number"| LoadSessions LoadSessions --> NoSessions NoSessions -->|"empty"| EXIT0 NoSessions -->|"sessions present"| APIRead APIRead --> ReadFail ReadFail -->|"rc != 0"| EXIT0_DIAG ReadFail -->|"rc == 0"| AlreadyHas AlreadyHas -->|"yes — idempotent"| EXIT0 AlreadyHas -->|"no"| EmptyBody EmptyBody -->|"empty body"| EXIT0_DIAG EmptyBody -->|"has content"| FormatTable FormatTable --> Concat Concat --> APIPatch APIPatch --> WriteFail WriteFail -->|"raised"| EXIT1 WriteFail -->|"success"| COMPLETE class START,EXIT0,EXIT0_DIAG,EXIT1,COMPLETE terminal; class ParseEvent,ExtractURL,LoadSessions,FormatTable,Concat handler; class NoURL,NoParts,NoSessions,ReadFail,AlreadyHas,EmptyBody,WriteFail stateNode; class ParseParts,APIRead,APIPatch newComponent;Color Legend:
●— modified subprocess calls and helperCloses #550
Implementation Plan
Plan file:
/home/talon/projects/autoskillit-runs/impl-20260327-205423-846457/.autoskillit/temp/make-plan/token_summary_appender_rest_api_plan_2026-03-27_000000.md🤖 Generated with Claude Code via AutoSkillit