Repository navigation
feat: keep an opt-in problem history for the Briefing - #165
Conversation
With `problem_history: 'true'`, each sync run collects the repository's issues and merged pull requests since the last update, plus each merged pull request's architecture findings from its review artifact, sends them to POST /v1/briefing and commits the returned ledger as problems.json on the orphan codeboarding/briefing branch. Every rule lives in the service; the Action only gathers facts and writes the answer. Off by default, and it never fails the run. Co-Authored-By: Claude Opus 5.5 <[email protected]>
CodeBoarding reviewStatus: 1 changed component See the full change in CodeBoarding. graph LR
n_action_support_scripts["action_support_scripts"]
classDef added fill:#1f883d,stroke:#0b5d23,color:#ffffff;
classDef modified fill:#bf8700,stroke:#7d4e00,color:#ffffff;
classDef deleted fill:#cf222e,stroke:#82071e,color:#ffffff,stroke-dasharray:5 3;
class n_action_support_scripts modified;
|
- Cut every issue body to 2000 UTF-16 code units, never splitting a surrogate pair; the oversize-only trimming is gone. - Stay under the service's caps by construction: 1000 open plus 1000 recently closed issues on a first run, 1000 plus 100 re-read later, 500 merged, 40 new plus 10 retried findings. - Send openIssues: every open issue number, read first in creation order, complete false past 5000, after a capped issue read, and on the conflict retry. - Ask again for findings of the 10 newest ledger merges whose findings were never read; send skipped envelopes so they count as read. - Re-read up to 100 stored open issues with an open linked pull request. - First-run caps are per pass, and issuesThrough never claims an issue a cap skipped. - Skip review artifacts over 50 MB, scan at most 20 pages of merged pull requests, give the service call 300 s, and stop the script after 10 minutes (a composite step cannot set timeout-minutes). - A ruleset or branch-name 422 on the ref update is a clear notice, not retried; a second lost race is a notice too. Co-Authored-By: Claude Opus 5.5 <[email protected]>
- Page a pull request's changed files through the shared `_nodes` pager, starting from the page MERGED_QUERY already returned, instead of a second hand-written cursor loop; `islice` keeps the 300-path cap from asking for a page past it. - Build issue, linked pull request and merged pull request facts as one literal each, with an `_author` spread for the optional login. - Pick a timeline event's pull request through one side table. - Read the ledger's ref and file under one 404 guard, and drop the unreachable assertion after the retry loop. Co-Authored-By: Claude Opus 5.5 <[email protected]>
- FakeGitHub gains add_review() for a run plus its review artifact and calls() for the GraphQL requests a test inspects; ledger(), numbers() and closed_node() replace literals repeated across tests. - Table-driven: the surrogate-safe cut, the 5000 open-issue boundary, first-run caps, read_architecture, main's never-fail outcomes and the three commit shapes (orphan, fast-forward, push-named). - The changed-paths test now also asserts no page past the 300-path cap is requested; the cap test asserts the uncapped run's issues. The new file passes against the code before the refactor, and the old file passes against the code after it. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Keep what a customer needs (opt-in, what is sent and its limits, what is written, permissions, never failing the run, how to turn it off) and drop the restatements: the Briefing's wording when it is off, and where the problem rules live, which the module docstring already says. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…d_features problem_history had its own copies of the GitHub request, the error-body message and the gzip-and-check step. closed_features now takes a method, payload, API base, token and timeout on _github, exposes _error_message and _gzip, and names what _check_size measured; problem_history uses them. Co-Authored-By: Claude Opus 5.5 <[email protected]>
A later run cut at 1000 issues recorded the last one read as issuesThrough. When more than 1000 issues share one update time, the next run starts on the same page and reads it again forever. A capped run now records at least the previous issuesThrough plus the overlap and a second, so each run moves on; ties left past the cap are lost. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ead again Re-reads picked the 100 issues whose stored pull request moved most recently, so those were read every run and the rest never were. They now take turns in number order, starting where the previous update stopped, which the commit count of codeboarding/briefing tells. The ledger head is read with that count in one GraphQL query instead of the REST ref. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…read Past 500 merged pull requests the scan stopped at the first 500 in update order, newest first, and said nothing. The service records the newest merge it is sent as where the next run starts, so the older ones were never read. A capped run now sends the 500 oldest, notes the cut, and the next run carries on from there. The README states the cap. Co-Authored-By: Claude Opus 5.5 <[email protected]>
…own reviews, within bounds Retrying unread findings could cost tens of requests a run for nothing: every unread merge of 56 days was tried, any workflow's runs on its head commit were listed, and each head commit took its own pulls call. Retries now stop at the review artifact's 14-day retention, list only CodeBoarding runs of pull_request events, and read the heads in one aliased query. A fork's pull_request run executes the fork's workflow, so it could upload a forged review artifact. Findings now come only from runs whose head repository is this one, and only when metadata.json names the pull request. metadata.json and closed/architecture.json are not read past 1 MB each, and an envelope that would take the request over the service's limit is left out (largest first) instead of skipping the whole update. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Without id-token: write, _bearer falls back to the run's GITHUB_TOKEN, which on the sync job holds contents: write. The problem history now skips the update with a notice to add id-token: write, before it reads anything. The input description, the README permissions and the failure hint list all five permissions, and the hint also fires when GraphQL refuses the repository or one of its lists (FORBIDDEN or INSUFFICIENT_SCOPES), which used to surface as a bare error. Co-Authored-By: Claude Opus 5.5 <[email protected]>
The full list costs a request per 100 open issues on every run, 20 for a repository with 2000, and only lets the service drop an issue that was deleted, transferred or made a discussion. It is now read by the first update of each UTC day (the ledger's updatedAt says which that is), and the other runs send it as incomplete, which drops nothing. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab8a13a8e1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # A bulk edit can give more issues than the cap one update time, and stopping at it would | ||
| # start every later run on the same page. Moving past the overlap loses ties past the cap. | ||
| stop = max(last or since, since + ISSUE_OVERLAP + timedelta(seconds=1)) |
There was a problem hiding this comment.
When more than MAX_ISSUES issues have the same updatedAt value, the first capped pass records that timestamp, and the next pass rereads the same earliest 1,000 issues because of the overlap. This max(...) then advances issuesThrough beyond the shared timestamp, so every remaining issue in that bulk update is permanently skipped rather than being picked up by later syncs. The continuation needs a stable tie-breaker or another way to retain progress without moving past unread timestamp ties.
Useful? React with 👍 / 👎.
| for n in _nodes(gh, MERGED_QUERY, variables, "pullRequests", pages=MAX_MERGED_PAGES): | ||
| if n is MORE: | ||
| notes.append(f"scanned the {MAX_MERGED_PAGES * 50} latest updated pull requests only") | ||
| break |
There was a problem hiding this comment.
If a bulk edit makes more than 1,000 historical merged PRs newer in updatedAt order than a recently merged PR, this branch stops before reaching the new merge. Because no scan cursor is persisted, every later sync starts from page one and examines the same 1,000 PRs, so the hidden merge and its architecture findings may never enter the history. Persist a continuation point or otherwise ensure a capped scan eventually visits later pages.
Useful? React with 👍 / 👎.
| closer = next( | ||
| ( | ||
| t["closer"] | ||
| for t in timeline | ||
| if (t or {}).get("__typename") == "ClosedEvent" | ||
| and (t.get("closer") or {}).get("__typename") == "PullRequest" |
There was a problem hiding this comment.
For an issue that was closed by PR A, reopened, and later closed by PR B, the timeline contains both ClosedEvents in chronological order, but next(...) selects PR A. The service therefore receives an obsolete closedBy relationship and can attribute the eventual fix to the wrong change; choose the latest qualifying closure event instead.
Useful? React with 👍 / 👎.
| listed = gh.json(f"/repos/{repo}/actions/runs?head_sha={quote(sha)}&event=pull_request&status=success&per_page=30") | ||
| runs = [r for r in listed.get("workflow_runs") or [] if _own_review(r, repo)] |
There was a problem hiding this comment.
When a head SHA has more than 30 successful pull_request workflow runs and its CodeBoarding run is not on the first page, this single request never sees it. Subsequent retries repeat the same first page until artifact retention expires, so the merge's architecture findings are silently omitted even though a valid artifact exists. Paginate the workflow-run results until the CodeBoarding candidates are found or the intended search limit is reached.
Useful? React with 👍 / 👎.
| retries = newest(m for m in _unread(ledger) if (_parse(m.get("mergedAt")) or retained) > retained) | ||
| retries = retries[:MAX_FINDINGS_RETRIES] |
There was a problem hiding this comment.
When more than MAX_FINDINGS_RETRIES ledger entries remain unread, this always retries the newest ten. If any of those entries has no usable artifact, it remains unread and is selected again on every sync, while older entries wait behind it and reach the 14-day retention cutoff first. This is especially likely after a run processes more than 40 merges, and it can prevent valid artifacts for older merges from ever being inspected; persist retry progress or rotate through the unread set.
Useful? React with 👍 / 👎.
What
A new input,
problem_history, off by default. When a repository sets it to'true', every sync run (a push to the analysed branch) keeps that repository's problem history for the CodeBoarding Briefing:problems.jsonfrom the orphancodeboarding/briefingbranch in the same repository.POST /v1/briefing, licensing-aws chore(main): release 1.7.0 #51), which returns the new ledger. All rules live in the service; this repository only collects and commits.codeboarding/briefing: one commit per update, never merged, runs no workflow. If another run moved the branch first, it rebuilds once on top of that.Nothing is sent or written without the input. The README's new "Problem history" section shows the permissions the sync job needs (
contents: write,issues: read,pull-requests: read,actions: read) and exactly what is sent and written. The step never fails the run.Tested
tests/test_problem_history.py. Black and actionlint are clean.CodeBoarding/CodeBoarding-evals: the first run created the orphan branch with 3 problems (endpoint on dev); a second run fast-forwarded it with 0 new problems.Merging this does not move
@v1: release-please holds the release until its release PR is merged. Release it only after #51 is in production.Nothing visible changes in this repository; what the Briefing shows is on webview #281.
Review fixes
issuesThroughnever claims issues a cap skipped.timeout-minutesis not allowed on a composite action's steps), artifacts over 50 MB are skipped, the merged scan is capped, and the service call times out at 300 s.CodeBoarding-evals.Second review fixes
id-token: writethe update is skipped with a notice. The input description and README list all five permissions.metadata.json, so a fork cannot plant them; retries stop when the review artifact has expired (14 days); artifact entries over 1 MB are not read; an oversized request drops the largest findings instead of the whole update.closed_features.py.CodeBoarding-evals; without OIDC it skips and sends nothing.🤖 Generated with Claude Code