Repository navigation
Make the runtime boundary check blame only a PR's own increase - #15153
Merged
Merged
Conversation
Collaborator
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
… import The ratchet needs to know which file and which API raised a count before it can say what to change. countText returns the Deno API keys and the counted std specifiers of one file, and countScope sums it, so the absolute totals are unchanged (checked on main: same five rows). scopesOf shares one predicate with countScope, so a changed-file list can be mapped to scopes with the same exts, exclude, include and skip rules. The new tests are characterisation tests of new pure functions: their first run failed at module load, which is not a RED step.
… replacements diffChanges turns a list of changed files into per-scope deltas and findings. Increases net out per Deno API and per std module before any file is blamed, so a renamed file or a moved call is not a finding. Only an exact API key in kRalDenoReplacements (build.os, build.arch: the ones src/deno_ral covers equivalently today) carries a replacement; std imports only ever carry a suggestion, because a name check cannot show that a wrapper exports what the file uses. The new tests are characterisation tests of new pure functions: their first run failed at module load, which is not a RED step.
A covered Deno API is the only error; everything else is a warning that says nothing needs to change in the PR and a maintainer accepts the count. The job summary carries the per-scope delta, the replacement to use, and the maintainer block with the expected post-merge counts, so the reviewer sees at review time that a baseline bump is needed. When main is already above the baseline and the PR adds nothing, the result is a notice, not a failure. The new tests are characterisation tests of a new pure function: their first run failed at module load, which is not a RED step.
A comment or string such as Deno.constructor made the per-API count read the inherited Object.prototype.constructor, so the scope total became a string and the delta NaN: the positive-delta guard then skipped every finding, including a covered Deno.build.os increase, and the PR passed. Deno.__proto__ was dropped entirely by the inherited setter. countText and the empty counts of diffChanges now use Object.create(null), so any identifier is an ordinary key.
Pull request runs check out the merge commit, so comparing absolute counts with the baseline blamed every open PR for an increase merged on main by another PR (#15136 after #15134). With --against HEAD^1 the PR is compared with the base branch tip and judged only on its own change; push runs keep the absolute baseline gate. gitChanges reads NUL-delimited git output through execProcess, splits renames into delete + add and checks every git call. --accept-increase replaces --accept-tests-increase and accepts any scope, since an uncovered increase in src needs a bump too. The new tests are characterisation tests of new functions: their first run failed at module load, which is not a RED step.
Plans that raise a count tell the executor to run --accept-increase now that the flag covers every scope, and the README lands 02c before any count-changing PR. The template carries the rule that a PR adding an equivalent deno_ral replacement adds its exact keys to kRalDenoReplacements in the same change, because a key added before the replacement works would fail contributor PRs with a hint that does not compile.
…tree A comparison with a long history changes enough files that the path arguments exceed the Windows command-line limit (os error 206 on a 1000-commit comparison), so delta mode failed before attributing anything. The existing set membership check already decides which paths exist at the base, so ls-tree no longer takes a pathspec. The regression test fails only on Windows: the Linux argument limit is about 2 MB, so there it passes with or without the change.
…line notice The notice compares the base (current minus the PR delta) with the baseline. Only a zero-delta test asserted notices, so computing the base as the current counts would have put a false "main is above" notice on every PR that raises a count while the suite stayed green. The raising-count renderDelta test now asserts no notice; with the base computed from current alone it fails.
… own The "main is above the baseline" notice reconstructs the base counts as current minus the PR delta. Comparing them with the PR's working-tree baseline gave a false notice for every PR that lowers a count and commits the lowered baseline (src 994 > 993). Delta mode now reads the baseline committed at the --against revision; a revision without one (before the ratchet) falls back to the working-tree baseline.
src/core/lib is imported by the browser-side YAML intelligence, so it cannot import src/deno_ral/platform.ts (it probes Deno in a try block instead). A new Deno.build.os there got the blocking error with a deno_ral hint that cannot compile in that code, and --accept-increase cannot clear a finding. Findings under src/core/lib/ now use the uncovered warning text.
…ort deno_ral The map covered both APIs in every scope, so a Deno.build.os added to the engine extension template (scope resources, user code) or to a plain JavaScript file (src-js) failed CI and asked for src/deno_ral/platform.ts, which neither can import. They are now errors only in src, package/src and tests; elsewhere the finding is the uncovered warning.
…eck can fail Verification 4 appended Deno.build.os to src/core/lib/text.ts and expected exit 1, but findings under src/core/lib/ are warning-only since that code is browser-shared. The recipe now uses src/command/utils.ts, where the covered error and the exit code still show.
cderv
force-pushed
the
rt/02c-ratchet-pr-delta
branch
from
October 22, 2026 13:58
9bfc775 to
861e72a
Compare
cderv
added a commit
that referenced
this pull request
Oct 22, 2026
The README asks whoever merges a plan's PR to move its row in "The sequence" from `planned` to `merged`. PR 02c landed in #15153, so the row now says so. This matters beyond bookkeeping: Phase 0 states that every PR that changes a Deno or std count lands after 02c, and the next plans (00a onwards) check their dependencies against this table at step 0.
cderv
added a commit
that referenced
this pull request
Oct 22, 2026
The plans themselves merged to main in #15133, so HANDOFF.md no longer points readers at the fork branch they were drafted on, and "How to resume" starts from main. PR 02 merged in #15135 and PR 02c in #15153. The README sequence row for 02 still said "in progress", and HANDOFF.md still named PR 02 as the first plan to open. Plans check their dependencies against that table at step 0, so it now says merged, and the handoff names 00a then 00b as the next plans in README order. docs: runtime migration, wrap-up checklist for agents The README already says who moves a row in "The sequence" and when, but a merge on GitHub runs no session, so the status column and HANDOFF.md drifted: PR 02 still read "in progress" after it merged. Findings made while executing one plan that change another plan's prerequisites were also left only in PR threads. An AGENTS.md in this directory is read by any agent working on the plans, the implementer included, so the reminder sits where the work happens. Findings go to README "Open decisions with an owner" rather than into the affected plan, which its step 0 re-reads anyway. The file is removed with the directory when the migration ends.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #15135, whose absolute comparison failed every open PR after a merge race: #15134 merged 20 minutes after the ratchet, its checks had run on a base without it, and its six new
Deno.*calls in two test files went unchecked.mainwent red (run) and, becausepull_requestruns check out the merge commit, #15136 failed for an increase it did not make (run).The baseline was raised on
mainin 8599166 to recover.The error text had a second problem: it always says "Application code must use src/deno_ral/", but
src/deno_ralhas noreadTextFile,writeTextFile,makeTempDirorenvyet, and only a thincall()for processes.A bug fix that adds
Deno.readTextFileSyncinsrcfailed CI with nothing to change.Plan: 02c-ratchet-pr-delta-and-guidance.md
Fix
Pull request runs compare the merge commit with its first parent (
--against HEAD^1,fetch-depth: 2) and judge only the PR's own change.Pushes to
mainand manual runs keep the absolute baseline gate.An increase fails a PR only for a
Deno.*API listed inkRalDenoReplacements: an exact key whosesrc/deno_ralreplacement is equivalent today (build.osandbuild.arch, in the scopes that can importdeno_ral; not inresources,src-jsorsrc/core/lib, which the browser-side YAML intelligence imports).Every other increase is a warning on the added line plus a job summary with the per-scope delta and a maintainer step: update the branch with
main, run--update-baseline --accept-increase, push the baseline change to the PR.The summary prints the expected counts after merge.
Increases net out per Deno API and per std module before any file is blamed, so a renamed file or a moved call is not a finding.
std imports are never errors: whether a
deno_ralwrapper replaces an import depends on the symbols used, on submodule semantics (path/posix) and on type exports, which a name check cannot establish.They get a suggested module from
kStdSuggestionsin the warning.--accept-increasereplaces--accept-tests-increaseand accepts any scope, since an uncovered increase insrcneeds a bump as well.A PR that adds a
deno_ralreplacement adds its keys tokRalDenoReplacementsin the same change (rule added to_TEMPLATE.md), which turns later increases of that API into errors.PR comments were not used: fork PRs have a read-only token, and
pull_request_targetis not worth its security surface for text the job summary already carries.Replaying the two incidents on Windows 11:
+6intests), the maintainer block withtests 832 / 177, exit 0.main: the noticemain is above tools/runtime-boundary-baseline.json (tests Deno.* calls 832 > 826); not caused by this PR, no warning, exit 0.Deno.build.osinsrc: the error with the added line number, exit 1.Checklist
I have (if applicable):
AI-assisted PR
Note: autonomous AI agents submitting PRs without human oversight are not permitted — see the Code of Conduct.