Repository navigation
Conversation
…ks 1-2 logged The executor now records per-task progress in three places (decided with Chris during PR 09's run): an "Execution log" section at the end of the plan file with commands and quoted output, ticked plan checkboxes folded into each task's commit, and br comments on the PR's issue. The README's "How to execute a plan" section documents the routine; the PR 09 row moves to in progress.
The execution-log routine already makes the plan file self-sufficient; this says when to use it (hand off at a commit point, resume per the checkboxes) and names it the fallback for executors without sub-agent tooling.
The log header states the commands ran under PowerShell, but the two recorded commands used the Bash env-prefix form (QUARTO_TESTS_NO_CONFIG=true pwsh ...), which fails before the test runner starts when replayed in pwsh. Record the actual PowerShell syntax ($env:QUARTO_TESTS_NO_CONFIG="true"; ...) per tests/README.md.
…elpers Add the fs section of the runtime layer (PR 09): read/write/stat/mkdir/ remove/rename/copyFile/readDir/symlink/readLink/realPath/chmod/utime/ truncate/open wrappers behind the layer's own FileInfo, DirEntry, FsFile, OpenOptions and WriteOptions types. readDir[Sync] return entries sorted by name in code-unit order; read errors are annotated with "\nPath: <path>" via deno_ral/errors.ts; canonicalPathSync calls the saved original realPathSync so it keeps resolving symlinks while core/deno/monkey-patch.ts is alive (until PR 25). Retype getFileInfoType and safeRemoveSync to the layer types so PR 10's rewrites type-check, move safeRemoveDirSync from originalRealPathSync to canonicalPathSync, and fix the header comment (encoding.ts copy-paste). Two adaptations from the plan's verbatim block, required by the vendored Deno 2.7.14 types: SymlinkOptions.type is required, so symlink[Sync] forwards an options object only when a type is given; truncateSync takes string only, so a URL path goes through toPathString first.
core/deno/monkey-patch.ts annotates Deno.readTextFile* errors with "Path: " + path, which for a file URL is the href. annotate() passed toPathString(path) (the filesystem path) to errorWithPath, so the suffix check never matched for URLs and a second "Path:" line was appended whenever the monkey patch was active. Use String(path) so both annotations are byte-identical and errorWithPath's idempotence holds; add missing-file URL cases to the annotation test (fails on the old code: two "Path:" lines).
Same defect fixed for Tasks 1-2 in 0c0886331: the Task 3 RED and GREEN entries recorded the Bash env-prefix form, and GREEN also carried a non-executable <all unit/ral/*.test.ts> placeholder, so neither command replays in the pwsh session the log header states. Record the PowerShell form and the actual glob command (pwsh passes the glob through; Deno expands it), verified to reproduce the logged 23 passed / 1 ignored result.
…ments
fs.test.ts had its own copy of tests/utils.ts withTempDir, and temp.test.ts
and errors.test.ts each defined the same code-carrying error helper; both now
use the shared versions (errorWithCode lives in tests/unit/ral/utils.ts).
errors.test.ts reached Deno.errors through (globalThis as any)["Deno"], which
only kept the access out of the runtime-boundary ratchet's `Deno.` count. It
now reads Deno.errors directly behind a typeof guard, so the test stays
skippable off Deno, and the tests baseline takes the one honest increase.
Comments in src/deno_ral cited migration plan PR numbers ("until PR 25",
"PR 27 moves this"), which go stale as soon as those PRs land. They now state
only the current reason, e.g. that Deno.* is looked up at call time because
core/performance/metrics.ts reassigns Deno.readTextFileSync.
…t, per-task pass The executor transcribes plan code blocks verbatim, and the plans' review checklists check behaviour only. PR 09's review found three defects that all came from its plan: a test helper duplicating tests/utils.ts withTempDir, Deno reached through globalThis to stay out of the ratchet count, and code comments citing plan PR numbers. A rough grep shows the same patterns across most remaining plans. Step 0 now audits each plan's code for those three points before execution (just in time, against current main, rather than a one-off sweep of plans that Step 0 re-checks anyway); the template's review checklist carries them as standing items; and the reviewer runs a reuse/simplification pass on each task's diff, with three greps for the whole branch before the PR opens. src/deno_ral is exempt from the globalThis rule, as it is from the ratchet.
The errorWithPath docstring and the annotate comment named the file that installs the read-annotation wrapper. The new wording holds while that wrapper is active and after its removal, so the removal-time grep gate in the migration plan gains two fewer comment hits. Behavior is unchanged: annotate still passes String(path), byte-identical to the wrapper's own annotation, so errorWithPath's idempotence is unaffected. The PR 09 execution log records this and the annotate URL fix (4bdd5f4ca).
PR 02d makes the ratchet count globalThis.Deno and globalThis["Deno"], but
leaves destructuring, aliases (`const d = Deno`), multi-word casts and
self./window. receivers uncounted. The review rule and its grep named only
the spellings the count now catches; `globalThis.*Deno` cannot match
`{ Deno } = globalThis` or `const d = Deno`. The grep now flags any added
globalThis and any bare `Deno` not followed by an access; `typeof Deno` and
prose hits are left to the reviewer.
Findings from a code review of the codemod; each now has a regression
fixture (21 tests pass, and --reverse on chrome-for-testing.ts produces
valid output).
- reverse: expand shorthand properties, e.g. `{ os }` becomes
`{ os: Deno.build.os }`, instead of splicing a dotted expression into
the shorthand position (invalid syntax).
- forward: a locally bound `Deno` (parameter, local, import, type) no
longer counts as the runtime global. resolveName with excludeGlobals
decides; getSymbolAtLocation cannot be used because it returns a
transient symbol with no declarations for the left of a qualified
name (`Deno.FileInfo`).
- forward: `import type * as fs` is no longer a merge target for named
type imports; rewriting it to `import type { X }` dropped the
namespace and broke existing `fs.X` uses.
- forward: an existing type-only binding of a layer export (inline
`type x` or declaration-level `import type`) now conflicts with a
value rewrite and is reported, instead of the merge adding a second
binding of the same name. Reporting matches the codemod's existing
collision philosophy and avoids promoting a deliberate type-only
import to a runtime import.
Follow-up review of the codemod found two shadowing errors, each now covered by a fixture. - reverse: introducing `Deno.*` at a reference did not check whether a local `Deno` captures it. An imported `statSync(p)` inside a function with a `Deno` parameter became `Deno.statSync(p)`, silently calling the parameter. Such a specifier now keeps its import and is reported in `left`, the same treatment as other uses reverse cannot express. - forward: the shadow check looked `Deno` up as value, namespace, type and alias at once, but TypeScript resolves by meaning. A value-only parameter named Deno does not hide the global namespace in `Deno.FileInfo`, and a local `type Deno` does not hide the global value in `Deno.readTextFileSync()`. Expressions now look up the value meaning and qualified type names the namespace meaning; aliases count for both, because an import named Deno may carry either. A forward dry run over src/ gives the same result before and after (816 replacements, 18 files with collisions, 2 skipped).
…d imports
A branch review found two more rewrites that produce invalid code, each
now covered by a fixture.
- forward: the guard that skips files patching Deno.* matched only a
plain `=` on `Deno.x`. `Deno.readTextFileSync &&= ...`, a nested
`Deno.build.os = ...` (the node the platform rule replaces), and
`delete Deno.env` all passed it and became writes to an imported
binding. Any assignment operator, `++`/`--`, or `delete` on either
node now skips the file. The skip message keeps its "assigns to"
wording, which the plan docs quote.
- reverse: `export { os }` and `export { cwd as currentDir }` name the
imported binding itself; there is no `Deno.*` spelling for them. The
import was dropped (leaving `os` unbound) or the specifier rewritten
to `export { Deno.cwd as currentDir }`. Such a specifier now keeps its
import and is reported in `left`.
Forward and reverse dry runs over src/ (all rules) are unchanged.
…ites
The write guard only looked at the immediate parent of `Deno.x`, so a
destructuring assignment such as
`({ readTextFileSync: Deno.readTextFileSync } = instrumented)`, an
array pattern `[Deno.build.os] = [...]`, or a `for (Deno.build.os of
...)` head passed it and became a write to the imported binding.
isWriteTarget now climbs through the array and object literals,
property values and spreads a pattern is written with, and counts the
node as written when the outermost literal is the left of `=` or the
head of for-of/for-in. A literal that is not a pattern (an argument, an
initializer) is still rewritten, which a negative fixture pins.
The forward dry run over src/ (all rules) is unchanged.
…ed receivers The write guard climbed only parentheses, so `(Deno.readTextFileSync as any) = r` or `Deno.build.os! = "linux"` passed it and became a write to the imported binding. It also only looked at `Deno.x` with a bare `Deno` receiver, so a patch through `(Deno as any).readTextFileSync = ...` or `Deno["readTextFileSync"] = ...` went unseen. That case is worse than a type error: a `const original = Deno.readTextFileSync` captured earlier in the file is rewritten to the layer wrapper, which forwards to the patched function, so the patch calls itself. Parentheses, `as`, `<T>x`, `x!` and `satisfies` are now one set of transparent wrappers, climbed both above a target and above the `Deno` receiver, and property and element access are both checked. Fixtures for each form were rewritten by the previous guard and are now skipped unchanged. The forward dry run over src/ (all rules) is unchanged.
A file that captures `const original = Deno.readTextFileSync` and then patches the method with `Object.defineProperty(Deno, ...)` passed the write guard. The codemod rewrote `original` to the layer wrapper, which forwards to the patched method, so the patch called itself. An alias (`const D = Deno; D.readTextFileSync = ...`) or `globalThis.Deno` hid the same patch. Rather than list every call that can mutate Deno, the guard now holds the codemod to the rule the PR review checklist already applies to src/: Deno is only reached as `Deno.x`. A file where the global Deno is an alias, an argument, a spread or a destructuring source, or is reached through globalThis/self/window, is skipped whole; `typeof Deno`, type positions and declaration names are not uses. For nested rules, `Object.assign(Deno.build, ...)` and the other Object/Reflect mutators also count as writes. What happens past an escape, or in another file, is still left to that review rule. The forward dry run over src/ (all rules) is unchanged: the only bare Deno uses are in the already-skipped monkey-patch.ts and in the excluded src/core/lib.
The plan said only a plain assignment to `Deno.<x>` skips a file. After the write-guard review rounds the codemod reports four skip reasons (writes, nested mutator calls, Deno used other than as `Deno.x`, Deno reached through globalThis/self/window), and the check is per file, with anything further left to the README rule against reaching Deno indirectly. Item 1 now states that, with the recursion it prevents.
…s recorded Steps 1-7 of Task 4 are ticked and logged, including the CRLF fixture normalization and the one-file delta in the dry-run count. A "Review of Task 4" section records the four write-guard rounds commit by commit, with the self-calling patch that motivates the guard and the dry-run figures that held unchanged across them, so the next session does not rediscover why the guard is broader than the plan's code block.
…callees
A shorthand property `{ Deno }` names both the property and the value it
reads, so the declaration-name exemption also exempted the read. With
`const holder = { Deno }`, a patch through `holder.Deno.readTextFileSync`
went unseen and an earlier `const original = Deno.readTextFileSync`
became the layer wrapper, which then called the patched method, itself.
A shorthand `Deno` now counts as a use, like any other escape.
The Object/Reflect mutator check for nested targets required a bare
`Object.assign` callee, so `(Object.assign)(Deno.build, ...)` passed. The
callee and its owner are now unwrapped from parentheses and type
assertions first.
The mutator table was a plain object, so an owner named after an
Object.prototype key (`constructor.inspect(Deno.readTextFileSync)`)
found the inherited `Object` constructor and the codemod threw on
`.includes`. It is now a Map.
`(Object.defineProperty)(Deno, ...)` was already skipped, since Deno as
an argument is an escape; a fixture pins it. The forward dry run over
src/ with all rules is unchanged.
The nested-target mutator check matched only a dotted callee, so `Object["assign"](Deno.build, ...)` was not recognised as a write and the file's `Deno.build.os` reads were rewritten. The callee may now be an element access with a string literal key. globalObjectDeno already read `o.x` and `o["x"]` the same way; both guards now share one memberOf helper. The forward dry run over src/ with all rules is unchanged.
…2526fd The fifth-round bullet lacked its commit, unlike the earlier rounds, and the bracket-callee follow-up was not logged.
memberOf accepted only quoted keys, so globalThis[`Deno`] and Object[`assign`] slipped past the escape and mutator guards. A file that captured Deno.readTextFileSync and then patched it through globalThis[`Deno`] was rewritten rather than skipped, which made the captured original the layer wrapper and the patched function call itself. A template literal without substitutions names its member as statically as a string literal does. Fixtures cover both callers of memberOf: the global-object path and the mutator callee.
memberOf checked the literal kind of the raw element-access argument, so
globalThis[("Deno")] and Object[("assign")] slipped past the escape and
mutator guards, as template keys did before the previous fix. A file that
captured Deno.readTextFileSync and then patched it through
globalThis[("Deno")] was rewritten rather than skipped, making the captured
original the layer wrapper and the patched function call itself.
The key is now unwrapped through the same transparent wrappers the receiver
and callee already are. Fixtures cover both callers of memberOf: the
global-object path and the mutator callee. The forward dry run over src/ is
unchanged.
…nth rounds The template-key round (3c973e12b) was not logged, so the parenthesized-key round (428714395) is the seventh rather than the sixth. Both now have a bullet and the intro counts seven rounds.
…f mapping it to Deno.realPathSync On stable branches Deno.realPathSync is the monkey-patched normalizePath, which neither resolves symlinks nor throws, so reversing to it would silently change semantics. --reverse now keeps the import and reports the binding in `left` with a hint naming originalRealPathSync from deno_ral/original-real-path.ts; the hint check runs before the re-export check so a re-exported canonicalPathSync keeps the hint too.
…atch files by path Seven review rounds each closed one bypass spelling and more remain (Reflect.get(globalThis, "Deno"), a computed globalThis[k], a comma-wrapped Object.assign((0, Deno.build), ...)); no file in the repo uses them. The guard is documented as best-effort and frozen, and kCodemodPatchFiles protects the two files that actually patch Deno (src/core/deno/monkey-patch.ts, src/core/performance/metrics.ts) as a backstop checked after the guard, so a caught file keeps the guard's reason.
…ction is tool-neutral The committed docs carried tracker IDs, local worktree paths and tracker instructions that only make sense on the maintainer's machine; progress tracking in README is now tool-neutral. Acceptance criteria, Verification and the review checklist are updated to the executed numbers (codemod suite 29 passed, ratchet baseline tests 874 -> 875 via the three-dot diff against the merge base). The ReadFileOptions review item is closed without a code change and the Context records why the layer's reads stay path-only.
…09 log The branch-review bullet named the removed path prefix verbatim, which the branch's own no-local-paths check (added lines matching C:/Users) flags. Reworded to "maintainer-local worktree paths".
The codemod fixture tests compare file bytes verbatim (assertEquals on the transformed output against the .expected.ts fixtures), and ts-morph inserts new lines as \n. With core.autocrlf=true on Windows, git checks tools/codemod/** out as CRLF: the blobs are LF, so nothing is committed back, but the comparison fails. A rebase surfaced this — replaying the branch re-checked the files out and 14 of 29 tests failed with pure line-ending diffs. src/** is already pinned text eol=lf for the same reason; this extends the rule to the one tools/ directory whose tests read file bytes exactly.
The two items HANDOFF held open for this branch are both done — the rebase onto main (02d, #15218) ran in this session, and the Task 4 codemod defects were fixed in the review rounds — so the "Pending on rt/09" section goes, the README sequence table marks 02d merged, and the plan's execution log records the rebase and its verification outputs, including the CRLF fixture failure the rebase surfaced and the .gitattributes pin that fixes it.
The rebase rewrote the branch's base, so the whole-branch review greps README "How to execute a plan" requires before the PR opens were re-run against upstream/main...HEAD and the results recorded in the execution log: no indirect Deno access outside the layer beyond an exempt typeof, no PR-number comments, and the one new test helper is the shared one the reuse rule asks for.
…deno_ral fs/temp/errors PR 09 task 5 (steps 2-4): run the codemod contract test in the runtime boundary workflow, document the --reverse backport procedure in the backport checklist, and point the TypeScript rules docs at the new fs/temp/errors layer surface. The backport procedure was verified on this branch's own fs.ts commit against upstream/v1.10 before documenting it: the patch applies with git apply --check, and the one unreverseable name (canonicalPathSync) is reported as `left` with the originalRealPathSync hint, as the doc describes. The check found one defect in the procedure as planned: reversing over --diff-filter=AM at the base revision crashes on files the commit adds, so the doc's rev() takes the filter (M at the base, AM after the checkout).
Lockfile step skipped (deno.lock arrives with PR 00b); changelog step waits for the PR number.
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. |
…09 task 5) Directories read through deno_ral are sorted by name in code-unit order, so discovery order stops depending on the file system once PRs 11-13 move the callers. References the draft PR number.
… docs The runtime-boundary workflow now also runs when only the codemod or its contract tests change. The backport recipe lists changes with --no-renames so renamed files are no longer dropped, reverses deleted TypeScript files in the base so their deletion applies against the stable branch's Deno.* form, and passes paths NUL-delimited so names with spaces survive. The rules no longer show a cwd import that deno_ral/process.ts does not export, and scope the no-new-Deno.* rule to callers outside deno_ral. The changelog names the directory order as character-code order: uppercase before lowercase only holds for ASCII names.
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
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.
The runtime migration (plans in
dev-docs/runtime-migration/, #15133) moves quarto-cli off Deno onto Node: everyDeno.*API the CLI uses gets a home in the runtime layersrc/deno_ral/behind runtime-neutral types, and callers are then moved mechanically, directory by directory. This is the layer's filesystem, temp-file and error surface, plus the codemod that moves the callers.No caller moves here, so the CLI's behaviour is unchanged and the ratchet baseline moves only in
tests(below). PRs 10–14 run the codemod oversrc/; its--reversemode produces theDeno.*form again for backports to stable branches.Plan: 09-ral-fs-and-temp-layer.md
The layer
src/deno_ral/fs.tsgains the plain filesystem calls (reads, writes, stat/lstat, mkdir, remove, rename, copyFile, symlink, truncate, open) behind its ownFileInfo(the 8 fields callers use, not Deno's platform-specific superset),DirEntry,FsFileand options types. Two behaviours fromcore/deno/monkey-patch.tscarry over exactly:"\nPath: <path>"annotation, idempotently, so nothing is annotated twice while the monkey patch is still loaded.makeTemp{Dir,File}{,Sync}(newtemp.ts) keep the 5-attempt retry, but only onAlreadyExists; the monkey patch retries on any error, which also hides real failures such as permissions.Two deliberate departures from the Deno shapes:
realPathSync. On branches that load the monkey patch,Deno.realPathSyncisnormalizePathand resolves no symlinks; exporting that name next to a symlink-resolving asyncrealPathwould pair opposite meanings under one name. The layer exportscanonicalPathSync(symlinks resolved, throws when missing), and the codemod mapsDeno.realPathSync(to it.readDirSyncreturns an array sorted by name in code-unit order andreadDirsorts before yielding. Several discovery loops (extension discovery, the theme list, MECA items) currently depend on the file system's listing order; the order becomes deterministic on every OS. The behaviour reaches users when PRs 11–13 move those callers; the changelog line is added with the PR number.getFileInfoTypeandsafeRemoveSyncare retyped fromDeno.FileInfo/Deno.RemoveOptionsto the layer's types so PR 10 type-checks when it moves their callers, andsafeRemoveDirSyncresolves throughcanonicalPathSyncinstead of the saved un-patchedDeno.realPathSync. The newerrors.tsholds the runtime-neutral predicates (isNotFound,isPermissionDenied,isAddrInUse,isConnectionError, …) that replaceinstanceof Deno.errors.X, each looking through one level ofcause, pluserrorWithPath.Reads look
Deno.readTextFileSyncup at call time rather than caching it, socore/performance/metrics.ts, which reassignsDeno.readTextFileSyncat runtime to record file reads, keeps working until PR 20b replaces that with a layer hook.The codemod
tools/codemod-deno-ral.ts(ts-morph, run withquarto run --dev) rewritesDeno.<call>(to the layer import and fixes up imports. Rulesfs,temp,env,process,platform; forward and--reverse;--dry-run,--dir,--file,--ral-dir.import typedeclarations stay type-only.Deno.*(assigns to it, or reaches the global other than asDeno.x: alias,globalThis/self/window,Object.assign(Deno.build, …)) is skipped, never half-rewritten. The guard went through seven review rounds with a fixture per escape; the two known patch files (core/deno/monkey-patch.ts,core/performance/metrics.ts) are additionally pinned by path as a backstop, and later PRs remove the pins as those files are migrated.kSrcExclusionsplussrc/core/liband the standalone scripts.--reversereports rather than guesses what it cannot express (left:lines). In particularcanonicalPathSyncis never reversed toDeno.realPathSync, which is the monkey-patchednormalizePathon stable branches; the report namesoriginalRealPathSyncfor the hand edit. The documented backport procedure was verified on this PR's ownfs.tscommit againstv1.10: the patch applies cleanly, with the oneleft:report as described.A dry run over
src/with all rules: 624 files scanned, 224 would be rewritten, 816 replacements, 18 files with collisions, 2 skipped (the two patch files).Tests
tests/unit/ral/(fs, temp, errors) is the compatibility contract the Node implementation of the layer (PR 27) must satisfy; the fs tests pin, among others, the single-Path:-annotation behaviour for both paths and file URLs.The codemod suite (
tools/codemod/, 29 tests driven by input/expected fixtures) runs in its own step in the runtime boundary workflow, not underrun-tests.sh, because it needsnpm:ts-morph; the fixtures are pinned to LF checkouts in.gitattributessince the tests compare bytes and ts-morph emits\n.Baseline
The ratchet moves only in
tests, 874 → 875:errors.test.tsreadsDeno.errorsbehind atypeof Denoguard, counted as one direct access. Accepted with--accept-increase; every other scope is flat.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.