Repository navigation
refactor: give repeated steps one owner and make previews refuse what writes refuse - #246
Merged
Merged
Conversation
…epeat The review of 2026-11-05 that produced adapter-simplification also traced copies outside the client adapters: SHA-256 digests, process runners, tools/ci path and scratch handling, a hand-written cover-profile parser, release publication's admission and asset comparison, the architecture scanner's test-only knobs, the store's four optional components, the updater's authorized GET, download and release resolution, and doctor's test-only inspector. Two tools/ci tests also bound a mise child at a fixed 20 seconds, which a hosted Windows runner exceeded on PR 245. It also found that `sync --dry-run` and `repair --dry-run` skip the transition check that `sync` and `repair` run. This Change gives each repeated step one owner and makes those previews refuse what their writes refuse. The command-line delta adds that scenario to "Every write can be previewed", and repository-organization adds one for steps that product packages and repository tools repeat. The verification tasks close before the archive, so the lane archives the Change itself. Change owner-consolidation.
Eight streaming copies opened a file, copied it into sha256.New and encoded the sum, and four byte copies encoded sha256.Sum256 of a buffer. internal/platform already computed the byte digest for NewFileSnapshot, and every adapter, internal/program and every tool may import it, so it now owns both forms beside that snapshot: platform.SHA256 for a buffer, which NewFileSnapshot uses, and platform.ReadSHA256 for a reader, which returns the read error unwrapped. The streaming callers are the Codex executable identity (codex/client.go), tools/ci's native input archive, client executables and Hermes installer (native_clients.go), the program inspection (program/inspect.go), the update checksum (program/artifact), and release construction's accepted notarization upload, native input package and tool image (notarization.go, input.go, execution.go), and provenance subjects. The byte callers are Claude Code's managed-settings digest, Codex's hashText and hashBytes with their callers and tests, Hermes' managed digest and Claude Desktop's configuration digest; those private helpers go. Each copy produced lowercase hexadecimal without a prefix, and so does the owner, so every digest, comparison and sidecar value is unchanged. Each caller keeps its own open, close and error wrapping, so the error text stays the same. Program inspection reads its size from the file offset after the digest, which equals the byte count that io.Copy returned. Provenance now streams a subject instead of reading it whole; os.Open and the reads return the same path errors that os.ReadFile did. Notarization keeps a bounded hasher for its upload entries, which compare a byte count as well. One-line Sum256 expressions that were not helper copies stay where they are. go test passes for internal/platform, the four adapters, internal/program, internal/program/artifact, tools/ci, tools/release/construction and tools/release/provenance. go vet passes for darwin, linux and windows, gofmt and golangci-lint (darwin and windows) report nothing, and the architecture gate reports no finding. Change owner-consolidation.
Release construction's executeTool and tools/ci's systemRunner both ran a tool with standard error in a native temporary file, replayed that file to the caller's standard error once the tool exited, and failed a run whose diagnostics carry a DiagnosticFailure marker; tools/ci also kept the first 64 KiB for its error text through diagnosticCapture. internal/platform/process now owns that step once, beside DiagnosticFailure: Runner.RunDiagnosed runs a plan as RunStream does, replays the diagnostics line by line, and returns their head (the same 64 KiB budget, exported as DiagnosticLimit), whether more followed and whether any line carries a marker. Its temporary file is removed before it returns, as both copies' files were. executeTool keeps its image resolution, timeout, environment and stdout capture for writers that are not regular files, and adds its "<tool> diagnostics prevent qualification" error from the returned Failure. tools/ci's systemRunner formats the same messages from the returned head: the run error with the diagnostics, "[diagnostics truncated]" past the budget, and "native gate diagnostics prevent qualification". diagnosticCapture and commandDiagnosticLimit go; the tests use process.DiagnosticLimit. runOwned keeps the call's timeout, plan and context error once and takes the run it performs, so systemRunner, systemOutputRunner and runAttached share it. systemOutputRunner keeps its single file for both streams, because its callers read the interleaved output and check it themselves. runAttached (the Linux Secret Service re-invocation) and tools/coverage's runner started their child with a bare exec.Command. They now run through Runner.RunStream, so these repository tools gain the process group or Windows Job that the other runners already have: interruption or return reclaims the child's remaining descendants. Their streams, environment, working directory and exit errors are the same; the coverage tool's failure lines now name the command, as in "run go: exit status 1". These are repository tools, not released programs. The diagnostics temporary file is named aigw-diagnostics-* for both tools, and construction's failure to create it reads "create command diagnostics", as tools/ci's did. A replay whose standard error write fails still reports the line it held, so tools/ci's error for a closed standard error now carries that text. go test passes for internal/platform/process, tools/ci, tools/release/construction and tools/coverage. go vet passes for darwin, linux and windows, and gofmt and golangci-lint (darwin and windows) report nothing. Change owner-consolidation.
repositoryFiles joined the checkout root to each path that git ls-files printed, and six callers turned the result back into relative paths: goToolSource, formatSource, checkSpelling, checkTOML, checkSecrets, and measuredSpecifications in size.go. It now returns the sorted slash-separated paths that git prints. inCheckout joins the root only for the tools that read their files from the command line or standard input and report them in that form: golangci-lint's package directories, lychee, markdownlint, the Mermaid check, actionlint and the workflow ledger, and scc with its budgets. formatSource and checkTOML pass the operating system's separator, as filepath.Rel gave them, and checkSecrets reads the same names through os.Root. The test of the requested checkout's index expects the relative paths. checkSecrets, checkSourceSize and checkRejectedSynonyms each created build/tmp, made their own temporary directory below it and removed it afterwards. scratchDirectory now does that once and returns the directory with the function that removes it; the synonym check keeps its "prepare synonym check" wrapping. requireTrackedLinkTargets keeps its own git ls-files --cached. It lists the index only, while repositoryFiles adds untracked files that are not ignored and drops missing paths, so reusing it would accept a link to an untracked document, which TestLinksRequireTrackedTargets refuses. Every check passes the same arguments, standard input and response files to its native tool. A shim that recorded each native invocation of check-format, check-markdown, check-mermaid, links, check-spelling, check-toml, check-secrets, check-source-size and check-workflows, for the root "." and for the lane's absolute path, recorded the same calls before and after this change apart from the random temporary-directory suffixes, and each check printed the same output. The list is sorted by its slash-separated form, the order git uses; on Windows the old order came from backslash-joined paths, which sorts differently only when a directory name is a prefix of a sibling name followed by a character between "/" and "\". go test ./tools/ci/... passes, go vet passes for darwin, linux and windows, and gofmt and golangci-lint (darwin and windows) report nothing. Change owner-consolidation.
readCoverage parsed Go cover profiles by hand: the mode line, three fields per row, the source range, non-negative counts, the merge of repeated blocks and the refusal of a repeated block whose statement count changes. cover.ParseProfiles, the parser behind go tool cover, does all of that. readCoverage keeps only the mode check, which now reads each parsed file's mode and keeps its message, and the per-package and total sums with their overflow check and the refusal of a profile without statements. realMain and the profile tests are its only callers. golang.org/x/tools moves from an indirect to a direct requirement at v0.52.0, the version go.mod already names. go mod tidy, run offline, changed only that line; go.sum is unchanged, and go mod tidy -diff and go mod verify pass. Renovate's gomod manager now sees it as a direct requirement. On the two retained real profiles in the main checkout, build/verification/coverage/profile.out (2026-11-04) and build/evidence/lane-20261105-secret-prompt-echo (2026-11-05), the coverage report that requireCoverage prints from the parsed result is byte-identical before and after this change: the same 71 per-package lines and 95.22% (18560/19492 and 18570/19502 statements). The run that reported 95.18% did not retain its profile. A repeated block whose statement count changes is still refused; the message is now the parser's "inconsistent NumStmt: changed from <n> to <m>" instead of "line <n> conflicts with repeated source range", and its test expects that text. Malformed rows are refused with the parser's messages, which quote the row instead of numbering it, and an empty file reads "profile contains no statements" instead of "profile is empty". Every malformed profile in the tests is still refused. go test ./tools/coverage passes, go vet passes for darwin, linux and windows, gofmt and golangci-lint (darwin and windows) report nothing, and the architecture gate reports no finding. Change owner-consolidation.
PublishGitHub, PublishGitLab and UploadGitLab each repeated the same admission before their first Forge request: publishInputs, VerifyMatrix, VerifyProvenance and verifyDistribution. admission.admit now runs that sequence once, in the same order and with the same refusals, and returns the release version; verifyDistribution's check is part of it. The GitLab commands still resolve their one credential first, and GitLabConfig.admission maps their configuration for both. verifyGitHubAssets and verifyGitLabAssets each downloaded every expected asset in artifact order, sent the credential only to the API's own authority and compared the bytes with the locally verified file. assetDownload.compare does that once. GitHub keeps its Accept header, its Bearer credential and its asset-set check; GitLab keeps its tag check, its link count and its Private-Token or Job-Token header. The messages read as before, with the Forge's name: "<Forge> release verification is missing asset <name>" and "<Forge> release asset differs from locally verified <name>". A GitHub asset URL that http.NewRequest refuses still returns that error; a GitLab URL has passed url.Parse and cannot reach it. commands.go built the same GitLabConfig for upload-gitlab and publish-gitlab; one closure builds it, still reading the CI variables when the command runs. githubRelease.ID was decoded and never read, and goes. lastPath returns path.Base of the parsed URL path; it differs from the old split only for a path that is empty or "/", which names no asset either way, and for one ending in two or more slashes, whose last element is now the asset name instead of an empty one. go test passes for tools/release and tools/release/publication, go vet passes for darwin, linux and windows, and gofmt and golangci-lint (darwin and windows) report nothing. Change owner-consolidation.
The architecture scan kept knobs that only its tests turned: - the repositoryAnalysis function table, which tests swapped to inject stage failures; - checkPackageChildrenWithReadDir, checkDecisionRecordsWithReadDir and collectGoFilesWith, which took os.ReadDir, filepath.WalkDir and filepath.Rel so that tests could replace them; - the --policy flag, its report-path rewriting and startsWithDotDot; - check_decision_records, check_semantic_names and unique_package_base_names, which the repository's policy sets true and only test policies left false. analyzeRepository now calls each check directly and unconditionally, in the same order, and the three checks call os.ReadDir, filepath.WalkDir and filepath.Rel themselves. run always reads <root>/.config/checks/architecture/policy.toml and reports that path, as the default flag value did. The three toggles leave the policy type and policy.toml; checkPackageBaseNames no longer takes the policy. The only invocation, tools/ci/quality.go's `--root .`, is unchanged, and on the lane the gate prints the same report before and after: ok, the same policy and root, and no finding. tools/ci reads only the tracked carrier classes from the same file and is unaffected. The tests now scan temporary Git checkouts with real files. writeRepository puts a fixture's policy at the root's policy path, with the empty Decision Record navigation that every scan now reads, and the fixture policy declares a documentation carrier for it. Stage failures come from the files: a navigation that is a directory, a missing package root or Go root, and layers without go.mod. The tests of an unreadable Decision Record directory, package root and Go root subdirectory close a real directory's read permission; like the repository's other permission tests they return early on Windows and skip as root. The tests of the removed flag, of a policy outside the root, of startsWithDotDot and of a disabled base-name rule go, and so does the injected filepath.Rel failure, which a walk below the root cannot produce. go test ./tools/architecture passes, go vet passes for darwin, linux and windows, gofmt and golangci-lint (darwin and windows) report nothing, go run ./tools/ci check-toml passes, and `go run ./tools/architecture --policy ...` is refused as an undefined flag. Change owner-consolidation.
TestMiseNpmEnvironmentIsBoundToThisRepository (both cache/ subtests) and TestBootstrapRejectsAmbientGoWithEmptyToolCache each stopped their mise child at a fixed 20 seconds. Hosted Windows exceeded that on PR 245 (run 42168528506, job 127730620265): the child had printed all four npm settings and had not exited at 20.0 seconds. One package-local constant, miseChildHangBound, now bounds both children at three minutes, the bound that ad44f04 gave the same kind of child in tools/release/main_test.go. It stops a hang; it is not a speed check. Each test still fails when its child passes the bound: the npm test on the context's deadline and the bootstrap test on "isolated bootstrap exceeded deadline". Both tests pass locally (the npm children completed in 0.2 and 0.14 seconds), go vet passes for darwin, linux and windows, and gofmt and golangci-lint (darwin and windows) report nothing. Change owner-consolidation.
The forwarding, retry, catalog and capability components were spelled out one by one: eight Snapshot fields, four path functions, and a block per component in Snapshot equality, BackupConverged, CaptureSnapshot, commitComponents, ConvergeVerifiedBackup, RestoreSnapshot and components.go's backUp, captureCurrent and sameCurrent. Each block made the same CaptureFileSnapshot, writeOptional or RestoreFileAtomicIfPostimage call and differed only in its path and message. One table now records each component's file suffix and the words its failures use, the Snapshot holds the current and backup generations as two arrays, and each step loops over the table once. Nothing a user can observe changes. Commits write in the same order (forwarding, retries, catalog, capabilities), restoration and verified convergence keep their own order (forwarding, catalog, capabilities, retries), the retry and capability components still read their backup before reporting a failed current read, and every message keeps its words. New tests in components_test.go pin the files that a commit and a verified convergence visit, in order, with mode 0600, every failure message and its compensation, every preimage refusal, every restoration conflict in order, and the order in which read failures are reported. They pass unchanged against the code before this commit. Outside the store only tools/release tests read Snapshot.Forwarding and Snapshot.Catalog; they now call the two accessors that replace those fields. No other package names a component field, and the path functions had no caller outside the package. Change owner-consolidation.
Four functions repeated one GET with an optional credential, a 2xx check and a size-limited JSON decode: publicReleaseMetadata, githubRelease, latestPrereleaseTagFromGitHub and latestTagFromGitLabAPI. Two repeated an authorized GET into a new 0600 file of at most 1 GiB: downloadGitHubAsset and downloadReleaseAssetFromGitLabAPI. All callers are in internal/program. releaseResponse now sends one releaseRequest (address, Accept, authorizer and the words of its failures), releaseMetadata decodes its document, and downloadAsset writes its file. Each caller states its own request. The requests send what they sent before: application/json and no credential for public metadata, application/vnd.github+json and the Bearer token for GitHub metadata, the Bearer token alone for a GitHub asset, and the Private-Token header alone for GitLab. GitLab's latest-release document keeps its 1 MiB limit and the others keep 4 MiB. Provider labels and every failure keep their words, including a GitHub asset transfer failure that carries no prefix of its own. GitLab still reads its token before it creates a request. The prerelease list still keeps the original 404 for any status that leaves the source available; a rate limit or a server failure still replaces it. downloadGitHubAsset had one caller, downloadGitHubReleaseAssets, which now states the GitHub asset request itself; its tests download through that caller. A new release_requests_test.go pins, for each of the seven requests, its headers, its full failure for a 418 answer, a 503 answer, a refused connection and a body that is not JSON, the file and its mode, and each metadata limit to the byte. It passes unchanged against the code before this commit. Change owner-consolidation.
Updater.Preview and resolvePeerReleases ran the same loop: skip a source that is not configured, read each source's latest tag, set aside the sources that cannot be reached, require one that answered, and refuse sources that disagree. Update and Preview were its only callers. One resolveLatestRelease now runs it for both. It takes the function that reads a source's tag and reports whether its failure leaves the source unreachable, and the words of the two failures that differ. Each caller keeps its own rule and words. Update reads through latestTagFromSource, which counts a GitLab source unreachable when glab or its API fallback is, and a GitHub source when the source is unavailable; it still reports "<provider> release metadata failed" and "no configured release source is reachable". Preview reads public metadata and also counts a 401, 403 or 404 as unreachable; it still reports "<provider> public release metadata failed" and "no configured release source publishes its metadata without authentication". The refusal of disagreeing sources keeps its words in both. resolvePeerReleases had no other caller and goes; its test calls the resolver as Update does. A new test pins the full failure of each operation for a failed source, for no reachable source and for disagreeing sources, and that one reachable source suffices. It passes unchanged against the code before this commit. Change owner-consolidation.
doctor took an inspector function through an unexported newCommand, which NewCommand, its one production caller, always called with clientStates. Only tests passed another one, or nil to report no client. The inspector type, its parameter and the forwarder go: NewCommand builds the command itself and every run observes the clients through app.InspectStatusClients. The nil case, which no production path took, goes with them. Without the function value in between, contextcheck sees that client inspection runs on its own context (app.InspectStatusClients starts one, as status and check also use it). The observation of the clients and the activation that they leave therefore moves from collectResult, which takes the command's context for its checks, into observeClients, which the command calls next. The order of every observation is unchanged: the credential backend, the checks, then the clients and the activation. Output, next actions and exit codes do not change. The tests now stub the adapter registry with adapter.NewRegistry, as internal/app/synchronization's tests do: Claude Code's adapter reports the projection state that a test needs, and a credential store whose metadata cannot be read makes a client unavailable. The tests that relied on the inspector observe real states: a configured and a deferred client, an invalid one, an unavailable one, and no client without a readable configuration. Doctor observes no endpoint, so the endpoint-checked, inference-checked, degraded and unclassified states that only the inspector could inject are no longer tested here. Change owner-consolidation.
prepareSyncPreview and repair's dry run built their plan from Synchronizer.Plan and app.PlanCredentialEntrypoint, which skip the validateTransition that `sync` and `repair` run before they write. A preview could therefore print a plan that the write then refused: `aigw sync --dry-run` printed one for a home without a configuration, which `aigw sync` refuses. Repair's convergence changes only the activation of the built-in clients, which the check accepts, so no repair preview of the built-in clients differs today; it now runs the check all the same. Both previews now call invocation.PreviewProjection, which client disable and uninstall already use, and embed invocation.Preview instead of declaring its projection and entrypoint fields again. A preview now refuses such a transition with the error of the write and reports no plan. An accepted preview keeps its human output, its JSON fields in the same order and its next_action. PreviewProjection plans the same clients and the same credential reader as before. sync.txtar and repair.txtar pass unchanged. Preview gains a Plans accessor because both commands render their own preview rows. Synchronizer.Plan stays for `use --dry-run`. A new test in each package refuses a transition that the write refuses, with --dry-run and with --dry-run --json, and checks the same error, no output and no write: an empty configuration for sync, and for repair a test adapter whose convergence moves Codex off the connection of its retry policy. Both tests fail against the code before this commit. One acceptance subtest that previewed sync in an empty home now gives the home a configuration whose client is not enabled, so it still shows the preview of disabled clients. CHANGELOG's [Unreleased] section records the fix. Change owner-consolidation.
A fresh-context review of part A found that giving runAttached and the coverage runner a process group of their own orphans what they start: the outer runner SIGKILLs the intermediate, and `go test` or the gate under dbus-run-session survives in its own group. The design no longer claims that these runners gain cleanup. They stay in the caller's process group, and the capture-and-check runner alone moves to internal/platform/process. Task 1.3 now names what the tools share: one scrubbed `git ls-files` invocation. The two tracked-file listings keep their own arguments, because the link index refuses an untracked target and repositoryFiles lists one. Change owner-consolidation.
5e9c86e moved runAttached in tools/ci and the runner in tools/coverage onto process.Runner.RunStream, which starts a child in a group of its own and sends that group SIGKILL when a signal arrives. Both children start nested runners. runAttached starts dbus-run-session with an inner tools/ci, and the coverage tool starts go test with its test binaries, and those keep some of their own children in groups of their own. A SIGKILL stops the intermediates before they can stop those children, so a gate runs on without its bus and a test's helper outlives the run. At the base they shared the caller's group, where a signal to the group reaches each of them and they stop what they started. The two runners return to their base code, an exec.Command with the same environment and streams, with one comment at each that says why. runAttached's child again ignores a command's directory and timeout, which its one call does not set. A Unix test per runner reads the child's process group and compares it with its own; giving the child its own group, as a check of the test, fails both. Windows has no process groups to pin. process.Runner.RunStream keeps its semantics, because the released program bounds its native credential subprocesses with it. RunDiagnosed and systemRunner's and systemOutputRunner's use of the native process owner stay. The design records this decision. go test -count=1 passes for tools/ci and tools/coverage, go vet passes for darwin, linux and windows, and gofmt and golangci-lint (darwin and windows) report nothing. Change owner-consolidation.
repositoryFiles and requireTrackedLinkTargets each built the same git ls-files call on the requested checkout, with the same removal of an inherited GIT_INDEX_FILE. gitListFiles now owns that invocation and splits its NUL-separated output. Each caller keeps its own arguments and error wrapping: repositoryFiles lists the index and untracked files that are not ignored, and the link check lists the index alone, so TestLinksRequireTrackedTargets still refuses an untracked document or directory and an ignored artifact. Those are the only two callers in tools/ci. The link check's arguments change order only, from --cached -z to -z --cached. The third copy, trackedFiles in tools/architecture/semantic_names.go, stays. No owner that both tools may import exists: internal/platform holds no Git code, tools/architecture may not import tools/ci, which sits in a higher row of the layer table, and it may not import a sibling in its own row. go test -count=1 ./tools/ci/... passes, go run ./tools/ci links . and check-toml . pass on the lane, go vet passes for darwin, linux and windows, and gofmt and golangci-lint (darwin and windows) report nothing. Change owner-consolidation.
The coverage tool kept a --policy flag and two swappable profile hooks that only its tests set. No consumer passes the flag: .ethos/profile.toml, tools/ci/quality.go and tools/ci/main.go run go run ./tools/coverage with --race, --profile-output or --tags, and git grep over .config, .github, .gitlab-ci.yml, CONTRIBUTING.md, AGENTS.md and docs finds no other use. The tool now always reads .config/checks/coverage/policy.toml from the checkout that it runs in, the path that the flag defaulted to, and loadPolicy takes no path. createCoverageProfile and removeCoverageProfile go; realMain calls os.CreateTemp and os.Remove itself, with the same name pattern, messages and exit codes. The tests run from temporary checkouts with real files. usePolicy writes a fixture's policy at the policy path and changes to that root. An unavailable temporary directory comes from TMPDIR, TMP and TEMP naming a missing directory, so it holds on every platform. A profile that cannot be removed stays open and loses its directory's write permission: Windows refuses to delete an open file, and other hosts refuse to delete an entry of a directory without write permission. That test still proves that a cleanup failure turns an otherwise passing run into a failure. The test of a failed profile Close goes, because a real, freshly created file that nothing has written cannot be made to fail its Close. The branch stays, untested, because a network or failing filesystem can still report an error there. go test -count=1 ./tools/coverage passes, go vet passes for darwin, linux and windows, and gofmt and golangci-lint (darwin and windows) report nothing. Change owner-consolidation.
Two knobs in the architecture gate served only its tests. exitFunc let TestMainCoversEntry call main without leaving the test process; main now calls os.Exit(run(...)) itself, and the tests exercise run, which holds all of main's behavior. TestMainCoversEntry goes with it; the other run tests cover what it checked. newReport took the policy path that production always passed as the constant, so it now records policyPath itself and takes only the root. Its 39 test calls drop their argument. The report keeps its bytes: the policy path is the same constant, which has no backslash for toPOSIX to replace. On the lane the gate prints the same report as before the knobs were removed. go test -count=1 ./tools/architecture passes, go run ./tools/architecture --root . reports no finding, go vet passes for darwin, linux and windows, and gofmt and golangci-lint (darwin and windows) report nothing. Change owner-consolidation.
e6976a0 gave the SHA-256 digest one owner, but three copies stayed. tools/ci's fileSHA256 and provenance's fileDigest each opened a file, called platform.ReadSHA256 and closed it, and catalogcheck's catalogProbe hashed its canonical metadata and encoded the sum. platform.FileSHA256 now owns the file form, beside ReadSHA256, and returns open, read and close errors unwrapped, as both copies did. The two local helpers go, and their callers (three in tools/ci/native_clients.go and one in provenance.go) call it directly. catalogProbe calls platform.SHA256. A local variable named platform in supplyClients becomes system so that the function can call the package. resolveImage in release construction did the same work with the same "read <path>: ..." text for an open and a read failure, so it uses FileSHA256 too. It now also reports a close failure, which it used to discard. Every output stays lowercase hexadecimal without a prefix. The other readers keep platform.ReadSHA256 on a file they open themselves. The Codex client and the update checksum word an open failure and a read failure differently in released messages; program inspection also needs the size, and notarization and native input go on to read the same file. Inline one-call expressions such as fmt.Sprintf("%x", sha256.Sum256(data)) stay where they are. go test -count=1 passes for internal/platform, tools/ci, tools/release/provenance, tools/release/construction and tools/codex/catalogcheck, go vet passes for darwin, linux and windows, gofmt and golangci-lint (darwin and windows) report nothing, and the architecture gate reports no finding. Change owner-consolidation.
testGitLabUnixMiseMirrorFailure stopped its mise install child at a fixed 15 seconds and failed on expiry, the same flake class that 8468ac9 removed from tools/ci. A package-local constant, miseChildHangBound, now gives that child three minutes, the bound that ad44f04 and 8468ac9 use. It stops a hang; it is not a speed check. The test still fails when the child passes the bound, and the child itself still fails fast on its own two-second HTTP limit. assertMiseConfigurationBoundary's mise env and mise run children in the same package had one minute and fail on expiry too, so they use the constant as well. The other bounds in the package stay: the mirror cleanup and the PowerShell argument witness start no mise child. go test -count=1 ./tools/ci/projection passes (the mirror children took about 0.1 seconds here), go vet passes for darwin, linux and windows, and gofmt and golangci-lint (darwin and windows) report nothing. Change owner-consolidation.
RunDiagnosed read each standard error line with ReadBytes, kept the whole line and copied it again for DiagnosticFailure, so a child that wrote a huge stream without a newline grew this process's memory without bound. The tools/ci code that it replaced streamed through fixed buffers. It now reads through a reader of DiagnosticLimit bytes with ReadSlice, so it holds at most one piece of a line at once. Every byte is still replayed to the caller in order. Measured once with a temporary test (not committed), a 128 MiB newline-free stream grew the heap by 639 MiB before this change and by 7 MiB after it. A line of up to DiagnosticLimit bytes is read and examined whole, as before. A longer line is examined piece by piece, so markers beyond its first 64 KiB still count. A marker across two pieces goes unseen, and a piece that starts with a sign or a word's tail can read as a marker, which errs toward refusal. Truncated is always set for such a line, because the head cannot hold it, and tools/ci refuses on Truncated as its old capture did. Release construction reads only Failure, so there the unseen case applies: only for a single line over 64 KiB whose one marker crosses a piece boundary. TestRunDiagnosedReplaysAndQualifiesTheCompleteDiagnostics gains two oversized newline-free streams, one of which carries a marker deep in the line, and every case now requires that no replayed write exceeds DiagnosticLimit. Raising the reader's size, as a check of the test, fails three of its cases. go test -count=1 passes for internal/platform/process, tools/ci and tools/release/construction, go vet passes for darwin, linux and windows, and gofmt and golangci-lint (darwin and windows) report nothing. Change owner-consolidation.
e512d30 replaced injected failures with directories that lose their read permission, and those three tests returned early on Windows, so the "read Decision Records", "read package root" and "walk go root" branches stopped running there. A file where a directory is expected does not reach them: each check stats the path or reads a file inside it first, and fails there. closeDirectory now denies listing on each platform. A Unix directory loses its read permission, as before. A Windows directory is held open without sharing, so that any other open of it fails, while Stat and a file inside it still work. The helper lists the directory itself and skips a test whose identity lists it anyway, which replaces the root check. The three tests run everywhere and check the branch's message. TestRelativePolicyFromRoot had lost its relative input when the --policy flag went. It now changes to the checkout and passes --root ., as tools/ci/quality.go does, and requires the report to name the policy path and the working directory as an absolute root. The error branch after filepath.Rel in collectGoFiles goes. WalkDir starts at the root joined with a go_roots entry, which policy validation keeps below it, and it yields only paths under that start, so Rel cannot refuse them. The Windows paths are compiled and linked here but have not run; the hosted Windows job is their first execution. go test -count=1 ./tools/architecture passes on darwin, go run ./tools/architecture --root . prints the same report, go vet passes for darwin, linux and windows, and gofmt and golangci-lint (darwin, linux and windows) report nothing. Change owner-consolidation.
RunDiagnosed examined a line longer than DiagnosticLimit in pieces of that size. A fresh-context review found two faults in that. A marker across two pieces went unseen, and each piece was read as if it began a line, so a cut word such as `error|s` or a sign just past a cut counted as a marker. Release construction reads only Failure, so a tool that exits 0 with such a line could qualify past a hidden warning, or fail on a false one. Such a line now counts as a failure outright, with no boundary logic to get wrong. tools/ci already refuses any truncated standard error, so only release construction changes: a tool that writes one line over 64 KiB to standard error no longer qualifies. That is the safe side for a release step, and the refusal is loud. The replay and its memory bound do not change. The table's oversized case now expects a failure, which the previous code did not report, and a new case keeps a line that fits the limit whole beside a truncated head. Change owner-consolidation.
closeDirectory skipped a test when the directory could still be listed. On Windows that would have hidden a helper that does not work: the three tests would skip, CI would stay green, and their failure branches would stay unexecuted, the gap that 550f32e set out to close. Only root lists a directory that denies listing, so the helper now skips for root alone and fails for any other identity. os.Geteuid returns -1 on Windows. Change owner-consolidation.
ClassifyClient gives doctor only Configured, Deferred, Invalid or Unavailable. With the test-only inspector gone, the Degraded, EndpointChecked and InferenceChecked labels and the default branch cannot be reached, in observeClients or in renderClients. Both now state the two outcomes that exist. A fresh-context review found them; DR-0009 and the coverage policy both ask for their removal. Output and exit status do not change. Change owner-consolidation.
84cfd0d counted a line over DiagnosticLimit as a failure, which was wrong: TestReleaseToolPreservesNativeImmediateExitDiagnostics pins that release construction accepts a long progress line and still finds a warning at its end. The full check caught it, so that behavior returns. The faults that the review found are fixed at their cause instead. A piece of a long line now keeps its unfinished last word, with any escape sequence before it, for the next piece, so a marker across two pieces is found and a word cut in two no longer counts. Only a line's first piece can carry a line-start sign. A word longer than the reach of 64 bytes can match only by its end, as in a whole line, so at most about 64 KiB of a line is held at once and replay does not change. New table cases cover a marker and a colored marker across two pieces, a longer word across two pieces, a sign inside an oversized line and a sign just past a piece edge. Removing the held word fails the first three, and removing the first-piece rule fails the fourth. go test passes for internal/platform/process and tools/release/construction. go vet and golangci-lint pass for darwin, linux and windows. Change owner-consolidation.
Groups 1 and 2 are done in the commits from e6976a0 to a946257. Task 1.1 now says what the code does: one SHA-256 owner for reader, file and byte digests, with one-call sha256.Sum256 expressions left in place. Task 1.7 names a package constant, because tools/ci and its projection package each hold their own and the Change adds no shared test support. At a946257 the local quality graph passed with 95.30% statement coverage, and mise run dependencies:check passed. The hosted Verify run 42202129777 passed on macOS, Linux and Windows at the same head, with the directory tests failing rather than skipping if a closed directory can still be listed. Change owner-consolidation.
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.
Summary
A review on 5 November traced steps that several packages and tools repeat. This lane (Change
owner-consolidation) gives each step one owner, removes hooks that only tests set, and fixes one preview defect. Non-test Go code shrinks by 199 lines.One behavior change, in the released program.
aigw sync --dry-runandaigw repair --dry-runnow run the transition check thatsyncandrepairrun, so a preview refuses what its write refuses, with the same error and no plan. Before,sync --dry-runprinted a plan for a home without a configuration. No repair preview of the built-in clients differs today. An accepted preview keeps its output, JSON fields andnext_action.Everything else keeps released behavior:
internal/platformowns the SHA-256 digests, andinternal/platform/processowns the capture-and-check run thattools/ciand release construction each copied. Inline one-call digest expressions stay.doctorloses its test-only inspector, andtools/architectureandtools/coveragelose their--policyflags and injection hooks.tools/cilists paths relative to the checkout and runs one scrubbedgit ls-files. Release publication has one admission, one asset comparison and one GitLab configuration builder.tools/coverageparses profiles withgolang.org/x/tools/cover, which moves from an indirect to a direct requirement at v0.52.0.tools/citests use a 3-minute bound. Hosted Windows exceeded the old 20 s bound on PR 245.Two decisions:
runAttachedand the coverage runner stay in the caller's process group. A group of their own would let the outer runner kill the intermediate and orphango testor the gate underdbus-run-session.tools/architecturekeeps its owngit ls-filescall, because the layer table bars it from importingtools/ciandinternal/platformhas no Git code. The link index keeps its index-only arguments, so it still refuses an untracked target.Review guide
Two places deserve the closest look.
internal/store: every file name, mode, order and message of the four components must survive the table.internal/platform/process/runner.go:RunDiagnosednow replays through a fixed 64 KiB buffer, so a long line is examined in pieces that keep their last word for the next piece. Markers must count as in a whole line, and memory must stay bounded.Validation
mise run checkpasses at a946257 (statement coverage 95.30%, above the 95% floor), and so doesmise run dependencies:check.doctorbranches. All are fixed.