Conversation
Size Report
Startup median (7 runs, lower is better):
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 12 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/contracts/src/runner-session-artifact.ts">
<violation number="1" location="packages/contracts/src/runner-session-artifact.ts:57">
P3: The contract's sanitizer is private, yet the writer at `packages/platform-apple/src/runner/runner-artifact-env.ts:48` still spells the same rule itself (`suffix.replaceAll(/[^a-zA-Z0-9._-]/g, '_')`). Two copies of the `[a-zA-Z0-9._-]` character set now govern on-disk session names; they're identical and the writer's pass is idempotent today, but any future edit to one copy silently renames sessions while the matchers keep the old bytes — exactly the drift this PR exists to prevent. Export `sanitizeRunnerSessionNameField` and have the writer consume it, or delete the writer's re-sanitize pass.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
|
|
||
| /** Characters a session name may carry; anything else is flattened, as the filesystem writer does. */ | ||
| function sanitizeRunnerSessionNameField(value: string): string { | ||
| return value.replaceAll(/[^a-zA-Z0-9._-]/g, '_'); |
There was a problem hiding this comment.
P3: The contract's sanitizer is private, yet the writer at packages/platform-apple/src/runner/runner-artifact-env.ts:48 still spells the same rule itself (suffix.replaceAll(/[^a-zA-Z0-9._-]/g, '_')). Two copies of the [a-zA-Z0-9._-] character set now govern on-disk session names; they're identical and the writer's pass is idempotent today, but any future edit to one copy silently renames sessions while the matchers keep the old bytes — exactly the drift this PR exists to prevent. Export sanitizeRunnerSessionNameField and have the writer consume it, or delete the writer's re-sanitize pass.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/contracts/src/runner-session-artifact.ts, line 57:
<comment>The contract's sanitizer is private, yet the writer at `packages/platform-apple/src/runner/runner-artifact-env.ts:48` still spells the same rule itself (`suffix.replaceAll(/[^a-zA-Z0-9._-]/g, '_')`). Two copies of the `[a-zA-Z0-9._-]` character set now govern on-disk session names; they're identical and the writer's pass is idempotent today, but any future edit to one copy silently renames sessions while the matchers keep the old bytes — exactly the drift this PR exists to prevent. Export `sanitizeRunnerSessionNameField` and have the writer consume it, or delete the writer's re-sanitize pass.</comment>
<file context>
@@ -0,0 +1,62 @@
+
+/** Characters a session name may carry; anything else is flattened, as the filesystem writer does. */
+function sanitizeRunnerSessionNameField(value: string): string {
+ return value.replaceAll(/[^a-zA-Z0-9._-]/g, '_');
+}
+
</file context>
|
Reviewed at 25f05bd. The rename to a process-identity contract looks correct. For realistic device ids, the runner argv and the disposal Before this goes further, one design question. The new contracts subpath is justified, because A pre-existing gap inside the invariant this PR names: a detached lease rewrites Smaller notes, not blocking: the writer still re-sanitizes the suffix with its own regex (runner-artifact-env.ts#L48); the round-trip test comment claims more than it pins, because a stem or prefix rename would pass on both sides (runner-xctestrun.test.ts#L366); and the dead Smoke Tests, Repo Guards, Coverage and Integration were still running. They all exercise this diff, so a failure there is likely related until shown otherwise. No live simulator run is needed: argv and |
…identity contract A runner launch is killed by matching `xcodebuild`'s argv with `pkill -f`, and that argv carries the per-session xctestrun's filename. The filename is therefore a process-identity contract, but it was spelled three times: inline in the session, inline in disposal, and again in the daemon-client timeout sweep. Disposal escaped the device id for regex without flattening it the way the writer's filesystem sanitization does, so a device id needing flattening built a cleanup pattern that could not match the file the writer had already written. Name it once. `runner-artifact-env.ts` now owns the stem, the suffix field order, the sanitization, and the `pkill -f` pattern derived from those same parts, and the writer, the session, and disposal all go through it. The name lives in the writer rather than a shared contracts module because the timeout sweep must not follow a rename: it ships separately, cannot know which version named a timed-out launch, and has to keep matching the names older writers used. So the sweep keeps a pinned literal, and its test proves those bytes still select both the owner-token and the pre-owner-token spelling. Placing the name in the module the Apple façades already evaluate also holds the eager-closure budget (#1960) at zero new edges. Verified byte-for-byte against the previous patterns for concrete, host-style, and flattened device ids. The only divergence is the flattened case, where the old pattern failed to match its own file. Also record three tooling constraints read from `xcodebuild(1)` and `simctl help`, and checked on Xcode 27.1: only `TEST_RUNNER_`-prefixed names cross into the test runner; `simctl launch --stdout/--stderr` resolve their paths inside the device's data container, so `--console-pty` is the console mode this host path can use; and xcodebuild re-synthesizes the attachment-lifetime keys from its own defaults, since a plan setting `keepNever` still builds an xctestrun carrying `SystemAttachmentLifetime=deleteOnSuccess`.
25f05bd to
0683a3d
Compare
This branch has not been deployed
1 participant
Summary
Follow-up to #2963 from the same inspection pass: what does runner process cleanup actually depend on, and is it declared anywhere?
A runner launch is killed by matching
xcodebuild's argv withpkill -f, and that argv carries the per-session.xctestrunfilename. The filename is a process-identity contract, but it was spelled three times: inline in the session, inline in disposal, and again in the daemon-client timeout sweep. Disposal escaped the device id for regex without flattening it the way the writer's filesystem sanitization does, so a device id needing flattening built a pattern that could not match the file the writer had already written.runner-artifact-env.tsnow owns the stem, suffix field order, sanitization, and the derivedpkill -fpattern; the writer, the session, and disposal all go through it. Runtime strings are byte-identical.The name lives in the writer, not a shared contracts module, because the timeout sweep must not follow a rename: it ships separately and has to keep matching names older writers used, so it keeps a pinned literal. That also keeps the eager-closure budget (#1960) at zero new edges — the first shape of this PR added a
contractsmodule and the gate rejected it on four entries.Also records three tooling constraints from
xcodebuild(1)/simctl help, checked on Xcode 27.1: onlyTEST_RUNNER_-prefixed names cross into the test runner;simctl launch --stdout/--stderrresolve inside the device's data container; and xcodebuild re-synthesizes attachment lifetimes from its own defaults (plan setskeepNever, built xctestrun carriesSystemAttachmentLifetime=deleteOnSuccess).9 files. Production +96/−13, tests +149/0.
Validation
Head
0683a3d46:pnpm check:affected --runpasses: eager-closure budgets 691 green, layering guard OK,check:fallow --base origin/mainclean in changed files, related Vitest green. Also passing:typecheck,lint,format:check.apple-runnerproject 627 tests andunit-coredaemon-client + eager-closure 805 tests green.xcodebuild .*AgentDeviceRunner\.env\.session-is asserted to still match both owner-token and pre-owner-token (session-<device>-<port>, shipped through v0.17.0) names.unit-ci, integration, and Swift runner lanes stay GitHub-authoritative.