Repository navigation
feat(hub): patch a policy file in place and open one pull request - #672
TheGreatAxios merged 2 commits into
Conversation
|
Review: needs fixes before ready. The branch also conflicts with main in apps/hub/src/integrations/app.ts; rebase locally.
|
6ffa01a to
197e2d7
Compare
|
Triage: Pass No changes needed from you. A maintainer will review this pull request. Notes:
|
corbits-triage
Bot
added
Triage: Needs Human Review
and removed
Triage: Waiting for CI
labels
7122abb to
b797749
Compare
![corbits-triage[bot]](https://avatars.githubusercontent.com/u/275831056?s=60&v=4){kind=small}
There was a problem hiding this comment.
Adds a hub route that patches a repository's existing policy file in place and opens or updates a single policy pull request, with a preview mode that writes nothing. The main defect is that the required-field check scans every built-in rule rather than only the ones a batch touches, so unrelated batches can be refused. Sentence regeneration also has two gaps: explicit text overrides the limit-derived sentence, and turning on an existing rule does not refresh its sentence.
P2 · A limit change with explicit text in the same batch keeps the typed sentence, not one from the limit · packages/triage-contracts/src/policy-patch.ts:0
Why it is wrong: applyOptions removes the rule from added whenever options contains text, so writeSentences skips it. The spec says a rule given a new limit takes its sentence from that limit, so this path produces a sentence that does not match the limit.
Triggering input: patchPolicyFile(FILE, repo, [{ kind: "rule-options", id: "max-lines-changed", options: { max: 200, text: "Custom wording." } }]) writes text: Custom wording. alongside max: 200, rather than "Flag pull requests over 200 lines changed."
Suggested fix: decide explicitly whether a supplied text should win. If the spec holds, regenerate the sentence whenever max is present and ignore or reject a conflicting text, e.g. if ("max" in options) added.add(id); before any text handling.
P3 · Turning on a built-in rule already in the file does not rewrite its sentence · packages/triage-contracts/src/policy-patch.ts:0
Why it is wrong: applyEnabled sets enabled only for rules already present and never adds them to added, so the sentence is not regenerated. The spec says a rule that is turned on gets its sentence from its limit, so a stale sentence is kept.
Triggering input: the file has max-lines-changed disabled with text "over 400" and max: 200. A batch with { kind: "rule", id: "max-lines-changed", enabled: true } leaves the text at "over 400".
Suggested fix: when enabled becomes true, add the rule id to added so writeSentences rebuilds its sentence from the current limit, e.g. if (enabled) added.add(id); alongside the setPair call.
| const field = requiredField(id); | ||
| if (field !== undefined && !item.has(field)) return `Rule ${id} is missing ${field}.`; | ||
| if (!item.has("reason")) return `Rule ${id} is missing reason.`; |
There was a problem hiding this comment.
P2 · Required-field check runs over every built-in rule, not only those the batch touched
Why it is wrong: missingRequired walks every built-in rule in the file, including untouched and disabled ones. The PR description says only a rule the batch turns on needs its reason, so this check is broader than the spec.
Triggering input: an existing file contains a built-in rule such as ci-green with no reason key, and the batch only toggles code review. The batch is refused with "Rule ci-green is missing reason."
Suggested fix: limit the check to rules that the batch enables or gives a new limit, for example by filtering the rule ids through the touched set before calling requiredField(id).
b797749 to
edb1ac8
Compare
corbits-triage
Bot
added
Triage: Waiting for CI
Triage: Ready
and removed
Triage: Ready
Triage: Waiting for CI
labels
There was a problem hiding this comment.
Adds a hub route that patches an existing policy file in place and opens or updates one policy pull request, with a preview mode that writes nothing. It can toggle rules, change rule limits and sentences, add or remove custom rules, and toggle code review. The most important defects are a stale-write race when committing the patched file and a rule-enable path that skips the required checks and sentence rewrite.
| async function policySource(tenantId: string, record: RepoRecord): Promise<{ path: string; text: string } | Response> { | ||
| const pull = await openPull(tenantId, record); | ||
| const found = (pull ? await findFile(tenantId, record, github.POLICY_BRANCH) : undefined) ?? await findFile(tenantId, record); | ||
| if (!found) return failure(409, "policy_file_absent", `${record.name} has no policy file. Open one with the export route before changing it.`); | ||
| const text = textOfFound(found); | ||
| if (text instanceof Response) return text; | ||
| return { path: found.path, text }; | ||
| } | ||
|
|
||
| /** | ||
| * Patches the existing policy file in place and opens the one policy pull request, or adds the batch to the open one, | ||
| * whose title and earlier batches stay. `preview` returns the file before and after and writes nothing. Refused when there is no file, or the batch or the file is refused. | ||
| */ | ||
| async function patchRoute({ repo, source, changes, preview }: typeof PolicyPatchBody.infer, member: Member): Promise<Response> { | ||
| const record = await grantedRecord(member, repo, source, "use"); | ||
| if (record instanceof Response) return record; | ||
| try { | ||
| const found = await policySource(member.tenantId, record); | ||
| if (found instanceof Response) return found; | ||
| const patched = patchPolicyFile(found.text, record.name, changes); | ||
| if (!patched.ok) return failure(422, "policy_file_invalid", patched.message); | ||
| if (preview === true) return Response.json({ before: found.text, after: patched.text }); | ||
| return await opened(member.tenantId, record, { | ||
| path: found.path, | ||
| text: patched.text, | ||
| title: `chore: update the Corbits Triage rules in ${found.path}`, |
There was a problem hiding this comment.
P2 · Patched text based on an earlier read is committed without stale-write protection
The file is read once in policySource and the new text is built from that read. The commit then fetches the current sha again and writes against it, so any commit pushed to the policy branch between those two reads is silently overwritten. This undercuts the promise that edits made on the open pull request are kept.
Trigger: a maintainer pushes to corbits-triage/policy after the patch route reads the file but before it commits.
Fix: take the sha from the same read used to build the patch, and pass it as the expected base when committing. Fail with a conflict if the branch head has moved, so the caller can retry on fresh content.
| if (isCatalogId(id)) { | ||
| const limits = Object.entries(options).filter(([key]) => !NOTE_KEYS.includes(key)); | ||
| if (limits.some(([key, value]) => JSON.stringify(doc.getIn(["rules", index, key])) !== JSON.stringify(value))) touch(touched, id); | ||
| if ("text" in options) touched.resentence.delete(id); |
There was a problem hiding this comment.
P2 · rule-options can enable a rule without the enable path's checks or sentence rewrite
enabled is in NOTE_KEYS and applyOptions writes it with setPair without calling touch(). A disabled catalog rule switched on this way skips the max/paths/reason check in missingRequired, and skips the resentence step, so its sentence can keep a stale limit. The spec requires a rule turned on to get its sentence from its limit.
Trigger: a batch { kind: "rule-options", id: "max-lines-changed", options: { enabled: true } } on a file where the rule is disabled with max 200 but its text says "over 400".
Fix: route enabled: true through the same path as the enable action, so required fields are validated and the sentence is regenerated from the limit.
| const edit = change.append ? { body: appendedBody(open.body, change.body) } : { title: change.title, body: change.body }; | ||
| const updated = pullOf(await json(gh, `/repos/${repo}/pulls/${open.number}`, { method: "PATCH", body: JSON.stringify(edit) })); |
There was a problem hiding this comment.
P3 · Unguarded read-modify-write of the GitHub pull request body loses concurrent batches
The new body is built from open.body as read earlier, then written with PATCH. GitHub applies no version check here, so when two patch requests overlap, one appended batch replaces the other.
Trigger: two patch requests run at the same time against a repository whose policy pull request is already open.
Fix: serialise appends per pull request, for example with a lock keyed on the repo and pull number, or re-read the body immediately before writing and retry if it changed.
edb1ac8 to
ec3762a
Compare
There was a problem hiding this comment.
Adds a hub route that patches an existing policy file in place and opens or updates a single policy pull request, with a preview mode that writes nothing. The edit preserves untouched lines, refuses changes when the host file moved, and appends each batch to the open pull request's description. Two defects need fixing: a fallback to the default branch makes retries fail permanently when the policy branch lacks the file, and GitHub write errors lose the host's reason text.
| /** The file on an open policy pull request's branch, else on the default branch. The branch wins so edits there survive. */ | ||
| async function policySource(tenantId: string, record: RepoRecord): Promise<{ path: string; text: string; sha: string } | Response> { | ||
| const pull = await openPull(tenantId, record); | ||
| const found = (pull ? await findFile(tenantId, record, github.POLICY_BRANCH) : undefined) ?? await findFile(tenantId, record); |
There was a problem hiding this comment.
P1 · Default-branch fallback makes every retry fail with policy_file_changed
When a policy pull request is open, the file is looked up on the policy branch first. If that branch has no policy file, the lookup falls back to the default branch, so readSha holds the default branch's sha. commitFile then compares that sha against the file on the policy branch, which is absent or different, and throws PolicyFileMoved. The caller is told to resend, but each attempt hits the same mismatch.
Trigger: an open corbits-triage/policy pull request whose branch has no .corbits-triage.yml or .yaml (or has it at a different path) while the default branch has one. Call the patch route.
Fix: when the pull request exists, only use the policy branch's file and its sha. If it is missing there, return a clear error or create the file on the branch from the default-branch content, and do not reuse the default-branch sha for the compare. For example, keep the sha paired with the branch it was read from:
const found = pull ? await findFile(tenantId, record, github.POLICY_BRANCH) : await findFile(tenantId, record);
| // GitHub refuses the write with 409 when the file moved off `sha` after the read above. | ||
| const res = await gh(url, { method: "PUT", body: JSON.stringify({ message: title, content: Buffer.from(text, "utf8").toString("base64"), branch: POLICY_BRANCH, ...sha }) }); | ||
| if (res.status === 409 && readSha !== undefined) throw new PolicyFileMoved(path); | ||
| if (!res.ok) throw new Error(`github PUT ${url} -> ${res.status}`); |
There was a problem hiding this comment.
P2 · GitHub write failures drop the host's error message
The contents PUT used to go through json, which included GitHub's error message after the status code. The new code throws github PUT <url> -> <status> with no body, so refusal() in github-refusal.ts shows "GitHub refused to … (HTTP 422)" and no reason. Bitbucket keeps its reason because it uses a different error type, so the two hosts now behave differently.
Trigger: GitHub rejects the commit with a message, for example from a branch protection rule. The user sees only the status code.
Fix: read the response body on failure and include its message in the thrown error, as the old json helper did. For example:
if (!res.ok) { const body = await res.text(); throw new Error(github PUT ${url} -> ${res.status}: ${body}); }
ec3762a to
4fad110
Compare
There was a problem hiding this comment.
Adds a hub route that patches a repository's existing policy file in place and proposes the change as a pull request, with a preview mode, optimistic-concurrency refusal on host changes, and repeated batches appended to the open pull request's description. The most important defect is that a no-op patch with no open policy pull request fails with a 502 instead of succeeding.
| return await opened(member.tenantId, record, { | ||
| path: found.path, | ||
| text: patched.text, | ||
| title: `chore: update the Corbits Triage rules in ${found.path}`, | ||
| body: patchPullBody(found.path, changes), | ||
| append: true, | ||
| readSha: found.sha, | ||
| }, "policy_file_patched"); | ||
| } catch (err) { | ||
| if (err instanceof github.PolicyFileMoved) return failure(409, "policy_file_changed", `${err.path} changed since it was read; send the changes again.`); |
There was a problem hiding this comment.
P1 · A no-op patch with no open policy pull request fails with a 502 instead of succeeding
Why it is wrong: patchRoute never checks whether the patched text equals the current file. When no pull request is open, proposePolicy recreates the branch from the default branch, so commitFile sees identical text and makes no commit. createPull then opens a pull request whose head has no commits relative to base. GitHub rejects that with a 422, no existing pull request is found, and createPull throws, so the route returns github_failed. The append path already returns early when nothing changed, but this path has no equivalent guard.
Trigger: a repository with a policy file and no open corbits-triage/policy pull request, sending a batch that changes nothing, for example turning on a rule that is already on or setting a limit to its current value.
Suggested fix: compare the patched text with the current file before proposing, and return success with no change when they match, as the append path does.
if (patched === current) return await opened(member.tenantId, record, { /* no-op */ });
| // GitHub refuses the write with 409 when the file moved off `sha` after the read above. | ||
| const res = await gh(url, { method: "PUT", body: JSON.stringify({ message: title, content: Buffer.from(text, "utf8").toString("base64"), branch: POLICY_BRANCH, ...sha }) }); | ||
| if (res.status === 409 && readSha !== undefined) throw new PolicyFileMoved(path); | ||
| if (!res.ok) throw new Error(`github PUT ${url} -> ${res.status} ${await errorMessage(res)}`.trimEnd()); |
There was a problem hiding this comment.
P2 · Commit failures now throw a plain Error instead of json()'s error
Why it is wrong: commitFile used to write through json(). It now calls gh directly and throws a generic Error on any non-ok status. The export and patch routes pass that error to githubFailed, which may depend on the status or error type that json() used to provide. Host refusals such as rate limits, 403, 422 ruleset refusals, or 409 conflicts could then be reported as generic failures. Whether this happens depends on how json() and githubFailed handle errors, which the diff does not show.
Trigger: the contents PUT is refused with 403, 422 or a rate limit, or with 409 when no readSha is given (the export route).
Suggested fix: keep the status on the thrown error, for example by attaching status and the host error code, and confirm githubFailed maps those cases the same way it did before.
if (!res.ok) throw Object.assign(new Error(github PUT ${url} -> ${res.status} ${await errorMessage(res)}.trimEnd()), { status: res.status });
| // GitHub takes no version on a pull request edit, so the body is read again just before it is written; a batch that | ||
| // writes between this read and the edit below is still lost from the body, though its commit is kept. | ||
| const latest: Pull | undefined = change.append ? await json(gh, `/repos/${repo}/pulls/${open.number}`) : undefined; | ||
| const edit = latest ? { body: appendedBody(latest.body, change.body) } : { title: change.title, body: change.body }; |
There was a problem hiding this comment.
P3 · Open pull request description grows with no limit
Why it is wrong: each batch that commits adds a full block to the open pull request's description, and nothing trims it. The description can reach the host's size limit. After that, every later patch fails on the pull request edit after its commit has already landed, which leaves the branch and the description out of step. Bitbucket is affected the same way through appendedBody.
Trigger: many patch batches are sent while one policy pull request stays open.
Suggested fix: cap the description size in appendedBody, for example by replacing older blocks with a summary or by refusing a batch once the limit is near, and check the pull request edit before committing so a failure does not leave a partial state.
const body = appendedBody(latest.body, change.body); if (body.length > MAX_BODY) throw new PolicyPatchError('pull_body_too_large');
4fad110 to
a1a7668
Compare
corbits-triage
Bot
added
Triage: Waiting for CI
Triage: Ready
and removed
Triage: Ready
Triage: Waiting for CI
labels
There was a problem hiding this comment.
Adds a hub route that patches a repository's existing policy file in place and proposes the change as a single pull request, appending each batch to the open pull request's description, with a preview mode that writes nothing. The main defect is that a batch which changes no rule can still commit a reformatting of the file, and retried batches can lose their description update after a failed body edit.
P3 · rule-options and rule disagree on turning on a catalog rule that is absent from the file · apps/hub/src/github/policy-file.ts:0
The rule change with enabled: true appends a missing built-in rule, but rule-options with enabled: true on the same absent rule returns 'not in the file'. The same intent is accepted in one shape and refused in the other.
Trigger: a batch sends {kind: "rule-options", id: "docs", options: {enabled: true, reason: ...}} for a catalog rule not yet in the file, where {kind: "rule", id: "docs", enabled: true} would succeed.
Suggested fix: make both paths share one lookup. When the id is a catalog id and enabled is true, append the rule in rule-options as well, or refuse both consistently with the same error.
| const patched = patchPolicyFile(found.text, record.name, changes); | ||
| if (!patched.ok) return failure(422, "policy_file_invalid", patched.message); | ||
| // A batch that changes nothing opens no pull request, which GitHub would refuse with no commits on its branch. | ||
| if (preview === true || (!found.pullOpen && patched.text === found.text)) return Response.json({ before: found.text, after: patched.text }); |
There was a problem hiding this comment.
P2 · No-op detection depends on the re-serialized YAML being byte-identical, so normalisation alone opens or updates a PR
The no-op check compares patched.text against found.text, but patched.text is produced by doc.toString(WRITTEN), which can reformat lines the batch never touched (quoting, indentation, flow spacing). A batch that changes nothing, such as setting a value the rule already holds, can therefore look like a change.
Trigger: a policy file whose formatting the YAML library rewrites, patched with options it already holds. With no open pull request, a PR that only reformats the file is opened. With one open, the reformat is committed and the batch's sentences are added to the description even though no rule changed.
Suggested fix: compare the parsed rule model before and after the patch, not the serialized text, and return early when nothing semantically changed. For example, compare the rule objects returned by the parser rather than patched.text === found.text.
| if (!open) await branchFromBase(gh, repo, base); | ||
| const committed = await commitFile(gh, repo, change); | ||
| // A batch that changed nothing adds nothing to the body. | ||
| if (open && change.append && committed === undefined) return pullOf(open); |
There was a problem hiding this comment.
P2 · A retried batch whose body edit failed never reaches the pull request description
The file commit lands before the pull request body is edited. If the body PATCH fails (502 or timeout), a retry of the same batch finds the branch already holding that text, so commitFile returns undefined. The early return if (open && change.append && committed === undefined) return pullOf(open); then skips the body edit. The batch stays in the commit but is permanently missing from the description, and nothing re-triggers the edit.
Trigger: a batch commits, the PATCH returns 502 or times out, and the same batch is sent again.
Suggested fix: do not skip the body edit when the commit was a no-op. Check whether the description already contains this batch's section and append it if not, for example if (open && change.append) return ensureSection(open, change);.
| await commitFile(bb, repo, base, Boolean(open), change); | ||
| const committed = await commitFile(bb, repo, base, Boolean(open), change); | ||
| // A batch that changed nothing adds nothing to the description. | ||
| if (open && change.append && !committed) return pullOf(open); |
There was a problem hiding this comment.
P2 · Bitbucket has the same lost-description retry: a failed body edit is never redone
The commit is made before the description PUT. When the PUT fails and the batch is retried, commitFile returns false because the branch already holds the text, and if (open && change.append && !committed) return pullOf(open); skips the description update. The batch's section is never added to the description.
Trigger: the description PUT to Bitbucket Data Center fails after the file commit succeeds, and the same batch is then retried.
Suggested fix: make the description update idempotent and run it even when the commit was skipped, checking whether the batch's section is already present before appending. This should match the GitHub fix.
| if (res.status === 409 && readSha !== undefined) throw new PolicyFileMoved(path); | ||
| if (!res.ok) throw await requestError(res, "PUT", url); |
There was a problem hiding this comment.
P3 · A file created on the branch between read and write is not reported as moved
readSha is null when the branch had no policy file, so the PUT is sent without a sha. If another commit creates the file before this write, GitHub rejects the PUT with 422 (sha missing) rather than 409. The code only maps 409 to PolicyFileMoved, so the caller receives a generic host failure (502) instead of policy_file_changed, which would have told them to resend.
Trigger: an open policy pull request whose branch has no policy file, while a concurrent commit adds the file before this PUT.
Suggested fix: treat a 422 with a sha-related error as a move when readSha was null, for example if ((res.status === 409 || (res.status === 422 && readSha === null)) ) throw new PolicyFileMoved(path);, and confirm the exact error message GitHub returns.
eb1a2c5 to
0ed7afd
Compare
There was a problem hiding this comment.
Adds a hub route that edits a repository's policy file in place and proposes the change as a single pull request, with preview, optimistic concurrency, and appending of batch descriptions to an open pull request. The main defects are that turning on a built-in rule overwrites its hand-written text, and that the new retry wrapping changes how export-route errors are reported.
| const named = new Set(sectionsOf(body).sections.map((section) => section.commit)); | ||
| // A message sent as a form field, as Bitbucket's is, comes back with CRLF endings. | ||
| const batches = commits.map((row) => ({ ...row, message: row.message.replace(/\r\n/g, "\n") })).filter((row) => !named.has(row.commit) && row.message.startsWith(`${title}\n\n`)); |
There was a problem hiding this comment.
P2 · Sections dropped for the size cap are re-added as missing by every later batch
bodyWith drops the oldest sections once the body passes PULL_BODY_MAX, but missingSections treats any batch commit the body does not name as missing. So each later batch re-appends the dropped sections as the newest ones and pushes out newer sections, so the description churns and loses its order.
Triggered when an open policy pull request has enough appended batches to pass 60,000 characters, and another batch is then applied.
Suggested fix: track dropped commits explicitly, for example by keeping a marker for sections that were trimmed, or by only treating a commit as missing if it is within the retained window and newer than the trim point.
| const updated = pullOf(change.append ? await appendSections(gh, repo, open.number, change, committed) : await retriedEdit(committed, function replaceBody() { | ||
| return json(gh, `/repos/${repo}/pulls/${open.number}`, { method: "PATCH", body: JSON.stringify({ title: change.title, body: change.body }) }); | ||
| })); |
There was a problem hiding this comment.
P2 · Export route's PR edit failures now surface as DescriptionNotUpdated, which the export route does not handle
The non-append path is now wrapped in retriedEdit, so any failure, including 403 rate limits or 404, is retried for about 700ms and then thrown as DescriptionNotUpdated. The export route still maps errors through githubFailed/bitbucketFailed, which do not recognise the wrapped error, so its status, rate-limit and refusal mapping is lost.
Triggered when the export route updates an open policy pull request and the PATCH is refused, for example by a rate limit.
Suggested fix: let the export route unwrap DescriptionNotUpdated to its underlying cause, or only retry on the errors that are actually transient.
| } | ||
| return createPull(bb, repo, base, change); | ||
| const path = `${repoPath(repo)}/pull-requests/${open.id}`; | ||
| await retriedEdit(committed, async function replaceDescription() { |
There was a problem hiding this comment.
P2 · Bitbucket export description edit is wrapped as DescriptionNotUpdated, so bitbucketFailed mapping is lost
replaceDescription failures, such as BitbucketFailed, become DescriptionNotUpdated. The export route does not special-case that error, so a host refusal is no longer mapped to the correct status.
Triggered when the export route runs against an open Bitbucket policy pull request and the description PUT fails.
Suggested fix: rethrow the original BitbucketFailed after retries are exhausted, or map DescriptionNotUpdated back to the host error in the export route.
| const index = ruleIndex(rules, id); | ||
| if (index < 0) continue; | ||
| const pack = readCheckPack({ ...emptyPack(repo), checks: { [id]: checks[id] } }, repo); | ||
| doc.setIn(["rules", index, "text"], `${catalogSentence(id, pack)}.`); |
There was a problem hiding this comment.
P2 · Turning on a built-in rule overwrites its own text with the catalog sentence
writeSentences rewrites text for every rule in resentence, and touch adds any catalog rule that is turned on, not only size rules. For a non-size rule the catalog sentence ignores any limit, so a disabled rule's hand-written text is replaced by the default wording. The change description only says a size rule's sentence is written from its limit.
Triggered by a file containing - check: focused with enabled: false and custom text "One focused change.". A patch {kind:"rule", id:"focused", enabled:true} makes the text "Check the change is focused on one thing."
Suggested fix: only rewrite text for size rules, or only when the existing text is absent.
doc.setIn(["rules", index, "text"],${catalogSentence(id, pack)}.);
| // GitHub refuses the write with 409 when the file moved off `sha` after the read above, and with 422 when a file | ||
| // appeared where none was read, as the write then names no sha. | ||
| const res = await gh(url, { method: "PUT", body: JSON.stringify({ message: commitMessage(change), content: Buffer.from(text, "utf8").toString("base64"), branch: POLICY_BRANCH, ...sha }) }); | ||
| if ((res.status === 409 && readSha !== undefined) || (res.status === 422 && readSha === null)) throw new PolicyFileMoved(path); |
There was a problem hiding this comment.
P2 · Any 422 on a new-file write is reported as a moved file
When readSha is null, every 422 from the PUT is mapped to PolicyFileMoved, so the caller is told to resend the changes. A 422 for another reason, such as a protected branch or a validation error, gets that misleading hint and hides the real cause.
Triggered when the policy branch has no file and GitHub refuses the PUT with 422 for a reason other than a file appearing, such as branch protection.
Suggested fix: inspect the 422 response body for the sha-mismatch message, and otherwise surface the GitHub error as-is.
if ((res.status === 409 && readSha !== undefined) || (res.status === 422 && readSha === null)) throw new PolicyFileMoved(path);
| // The description is read just before it is written; GitHub takes no version on the edit, and the hub runs one | ||
| // batch per repository at a time. | ||
| const latest: Pull = await json(gh, path); | ||
| const listed: PullCommit[] = await json(gh, `${path}/commits?per_page=100`); |
There was a problem hiding this comment.
P3 · GitHub commit listing reads only the first 100 (oldest) commits
GitHub's pull request commits endpoint returns commits oldest first, and only one page is read. Once the branch has more than 100 commits, a lost section from a newer commit is never found and never repaired, apart from the current commit.
Triggered by a policy pull request with more than 100 commits where an earlier description edit failed.
Suggested fix: follow pagination until all commits are read, or read the newest commits first.
const listed: PullCommit[] = await json(gh,${path}/commits?per_page=100);
| const index = ruleIndex(rules, id); | ||
| if (index >= 0) { | ||
| if (enabled && isCatalogId(id) && doc.getIn(["rules", index, "enabled"]) === false) touch(touched, id); | ||
| setPair(doc, ["rules", index, "enabled"], enabled); |
There was a problem hiding this comment.
P3 · Turning on an already-enabled rule writes enabled: true and counts as a change
A rule with no enabled key is already on. setPair compares undefined with true and writes the pair anyway. The parsed content then differs, so changed is true and a commit and pull request are made for a no-op request.
Triggered by a batch {kind:'rule', id:'ci-green', enabled:true} where ci-green has no enabled key.
Suggested fix: treat an absent enabled as true when comparing, and skip the write when the effective value already matches.
setPair(doc, ["rules", index, "enabled"], enabled);
| const rules = rulesOf(doc); | ||
| if (!rules) return MIGRATION; | ||
| const index = ruleIndex(rules, id); | ||
| if (index < 0) return isCatalogId(id) ? missing(id) : unknown(id); |
There was a problem hiding this comment.
P3 · rule-options with enabled refuses an absent built-in rule, while rule with enabled adds it
applyOptions returns missing for an absent catalog rule before it reaches applyEnabled, so a turn-on sent as options is refused. The same turn-on sent as a rule change appends the rule. The options path is documented as held to the same checks as the rule path.
Triggered by a batch {kind:"rule-options", id:"docs", options:{enabled:true, reason:"..."}} against a file with no docs rule. It is refused with "Rule docs is not in the file.", while {kind:"rule", id:"docs", enabled:true} adds it.
Suggested fix: let applyOptions fall through to applyEnabled when enabled is true and the rule is a catalog id, so both paths behave the same.
if (index < 0) return isCatalogId(id) ? missing(id) : unknown(id);
corbits-triage
Bot
added
invented-fallbacks: check
Triage: Ready
and removed
Triage: Ready
labels
0ed7afd to
fe342d7
Compare
corbits-triage
Bot
added
Triage: Waiting for CI
Triage: Ready
and removed
Triage: Ready
Triage: Waiting for CI
labels
There was a problem hiding this comment.
Adds a hub route that patches an existing policy file in place and opens or updates one policy pull request, supporting rule toggles, setting changes, custom rule add/remove, and code review on or off. The main defect is that built-in rules added via add-rule skip the sentence rewrite and required-field check, so the file can contradict the limit.
Not reviewed in full: 2 files, over the size limit.
packages/triage-contracts/src/policy-patch.tspackages/triage-contracts/src/policy-rules.ts
| function applyAdd(doc: Document, rule: PolicyRule): string | undefined { | ||
| const rules = rulesOf(doc); | ||
| if (!rules) return MIGRATION; | ||
| const id = fileRuleId(rule); | ||
| if (ruleIndex(rules, id) >= 0) return present(id); | ||
| appendRule(doc, rule); | ||
| return undefined; | ||
| } |
There was a problem hiding this comment.
P2 · A built-in rule added with add-rule skips the sentence rewrite and the required-field check
applyAdd takes no touched set and never calls touch, so a catalog rule added through add-rule keeps the text the caller sent even when its limit says something different. It is also skipped by missingRequired.
Trigger: send {kind:'add-rule', rule:{check:'max-lines-changed', text:'Flag pull requests over 400 lines changed.', max:200, ...}}. The file then says 400 while the limit is 200, and a missing reason or limit is not reported.
Fix: route catalog rules added through add-rule through the same sentence generation and missingRequired checks used by the rule/rule-options path, and mark them as touched so the sentence is derived from the limit.
| } | ||
| return createPull(gh, repo, base, change); | ||
| if (!open) return createPull(gh, repo, base, change, committed); | ||
| const updated = pullOf(change.append ? await appendSections(gh, repo, open.number, change, committed) : await json(gh, `/repos/${repo}/pulls/${open.number}`, { method: "PATCH", body: JSON.stringify({ title: change.title, body: change.body }) })); |
There was a problem hiding this comment.
P2 · Export over an open patch pull request drops its sections, so the next patch re-adds every earlier batch
The non-append export path replaces the description with the export body, which removes the [//]: # (corbits-triage added …) markers. The next append edit runs missingSections, which treats every branch commit whose message starts with the patch title and is not named in the body as missing. Those are all earlier batches, so their sections are duplicated and the body grows toward the cap. Bitbucket's non-append path overwrites the description the same way.
Trigger: a patch batch opens the policy PR, the export route then runs against the same repository, and another patch batch is sent.
Fix: have the non-append path keep the existing patch sections (or record the commits it has covered) so missingSections does not treat them as lost.
| await retriedEdit(committed, async function appendMissing() { | ||
| const latest = Pull.assert(await send(bb, path)); | ||
| // Data Center lists the newest commit first. | ||
| const listed = Commits.assert(await send(bb, `${path}/commits?limit=100`)).values.toReversed(); |
There was a problem hiding this comment.
P2 · Bitbucket lists only the first 100 commits, so a lost section older than that is never added
appendSections reads one page with commits?limit=100 and ignores isLastPage. The GitHub version pages through all commits with jsonAll and is tested past 100 commits. On Bitbucket, a batch commit beyond the first page whose section was lost is never found, so its change never reaches the description.
Trigger: the policy pull request has more than 100 commits on its branch and a section for an older batch commit is missing from the description.
Fix: page through the commits until isLastPage is true, as the GitHub implementation does, and add a test past 100 commits for Bitbucket.
Apply a batch of rule changes on the YAML document so comments and untouched lines stay, then open the one policy pull request, or add the batch to the open one's body under its title.
fe342d7 to
216b10c
Compare
corbits-triage
Bot
added
Triage: Waiting for CI
Triage: Ready
and removed
Triage: Ready
Triage: Waiting for CI
labels
There was a problem hiding this comment.
Adds a hub route that patches an existing policy file in place and proposes the change as a pull request, with batches appended to the open PR's description, plus a preview mode and contract checks for rule edits. The most important defects are that built-in rules added with custom text get their text overwritten, and that the description-update path can both exceed GitHub's body limit and mishandle errors.
Not reviewed in full: 2 files, over the size limit.
packages/triage-contracts/src/policy-patch.tspackages/triage-contracts/src/policy-rules.ts
| const id = fileRuleId(rule); | ||
| if (ruleIndex(rules, id) >= 0) return present(id); | ||
| appendRule(doc, rule); | ||
| if (isCatalogId(id)) touch(touched, id); |
There was a problem hiding this comment.
P2 · Built-in rule added with its own text has that text overwritten
Why it is wrong: applyAdd calls touch() for catalog ids without marking the rule's own text as kept, so writeSentences replaces the supplied text with the catalog sentence. applyOptions already marks ownText for text, so this path is inconsistent with the rest of the module.
Triggering input: an add-rule for a built-in check such as docs that carries a custom text. The written file gets the catalog sentence instead of the text supplied.
Suggested fix: when a text is supplied on add, mark the rule's own text as kept, for example by passing the same ownText flag used in applyOptions, before calling touch(touched, id). Add a test that adds a built-in rule with custom text and checks the file keeps it.
| for (const delay of EDIT_DELAYS_MS) { | ||
| try { | ||
| return await edit(); | ||
| } catch { | ||
| await Bun.sleep(delay); | ||
| } | ||
| } |
There was a problem hiding this comment.
P3 · Description edit retries every error and turns rate limits and auth refusals into description_not_updated
Why it is wrong: retriedEdit catches every error, including 403, 404, 429 and rate-limit refusals. It sleeps and retries, then wraps the final error in DescriptionNotUpdated, so the route answers 502 description_not_updated where it should return githubLimited or the host's refusal.
Triggering scenario: GitHub rate-limits or refuses the PR read or PATCH after the commit has already landed.
Suggested fix: retry only transient failures such as network errors and 5xx responses. Rethrow 403, 404, 429 and rate-limit errors immediately so the route can map them to their proper responses.
| const room = PULL_BODY_MAX - joined(opened, [...sections.slice(0, -1), { ...newest, text: "" }]).length - 2; | ||
| return joined(opened, [...sections.slice(0, -1), { ...newest, text: `${newest.text.slice(0, Math.max(0, room - TRUNCATED.length))}${TRUNCATED}` }]); | ||
| } |
There was a problem hiding this comment.
P3 · Body cap can be exceeded once markers alone pass PULL_BODY_MAX, and edits then fail for good
Why it is wrong: trimming removes only section text and keeps every marker, and markers grow with each batch. Once the opening text plus markers exceeds the cap, room goes negative and the result is still over the limit. Past GitHub's 65,536-character limit, every later edit is refused.
Triggering scenario: a long-lived policy PR that receives around a thousand batches, or one whose opening description is already very long.
Suggested fix: bound the number of retained sections, or compact older markers, and clamp the result so the body never exceeds PULL_BODY_MAX. Add a test with many batches that checks the final body length.
| return bodyWith(current, committed === undefined ? [] : [{ commit: committed, text: change.body }]); | ||
| } | ||
|
|
||
| /** A section for each of the branch's batch commits, oldest first, that the description does not name yet. */ |
There was a problem hiding this comment.
P3 · An export that commits nothing leaves the old body when the description holds batch sections
Why it is wrong: when committed is undefined, replacedBody returns bodyWith(current, []), which is the old description unchanged. The export's new body is dropped even though the title is replaced, so the title and body disagree.
Triggering scenario: an export to an open policy PR whose description has patch sections, where the branch already holds the exported text.
Suggested fix: when nothing was committed, still write the export's body, for example by building the body from change.body with the existing sections kept, rather than returning the old body unchanged. Add a test that exports to an open PR with batch sections and checks the body is replaced.
…ient edits Retry a description edit only on 429, 5xx or no answer; any other refusal is the host's error. Drop the oldest section markers behind one line when they alone pass the cap, so an edit never fails on size. Keep the text a built-in rule is added with. Serialize export with patch per repository, reuse listPrCommits, and remove unreachable rules-list branches and unused exports.
What changed
The hub gets a new route that changes a repository's existing policy file and proposes the change as a pull request. It can turn rules on or off, change a rule's settings, add or remove a custom rule, and turn code review on or off.
The edit is made on the file itself, so lines that were not asked about keep their content and comments. The YAML library may normalise their spacing. When the policy pull request is already open, the file is read from its branch so edits made there are kept. Each new batch of changes is added to that pull request's description and its title stays as it is.
A rule that is added, or given a new limit, gets its sentence from that limit, for example "Flag pull requests over 200 lines changed". A rule turned back on keeps its own wording. The caller must give its reason, and a size or path rule its limit, or the change is refused and the missing part is named. A rule's check and id cannot be changed this way.
If the file changes on the host between the read and the commit, the change is refused so it can be sent again, and nothing is overwritten.
A batch that leaves the file's content as it was writes nothing. If the pull request's description cannot be updated after a few tries, the route says which commit landed, and the next batch adds the missing part.
A preview returns the file before and after the change and writes nothing.
GitHub and Bitbucket Data Center share the same pull request code, and both add to the open pull request's description in the same way. The route test runs against the GitHub fake, and the Bitbucket pull request test covers the description being added to.
The user manual is unchanged because no screen changed. Nothing here judges or reviews a pull request, so there is no eval result.
How verified
The hub and contracts tests pass, including new checks that untouched lines keep their content and comments, that a size rule's sentence matches its limit when it is turned on or its limit changes, that changing a rule's check or id is refused, and that a second batch is added to the open pull request's description without retitling it. The typecheck and lint both pass.
Closes CL-11575