-
Notifications
You must be signed in to change notification settings - Fork 785
fix(windows): make the suite pass on an unelevated Windows checkout #1881
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
627eecf
1828cb1
9122d5e
5a4d968
dad5348
1b8c399
960c7a9
c388221
5c6be04
7563d35
079f417
e2f9670
54c5a8f
71f69ed
51857c9
a6de95a
0f4b95b
f3a6120
e7a71c1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,8 @@ | |
| * them here breaks that cycle. `inject.ts` imports them back and re-exports the | ||
| * two public predicates, so external callers see no change. | ||
| */ | ||
| import { parseTomlString } from "./paths"; | ||
|
|
||
| export const OCX_SECTION_MARKER = "# Auto-injected by opencodex"; | ||
|
|
||
| export function isRootOpenaiBaseUrlLine(line: string): boolean { | ||
|
|
@@ -16,7 +18,11 @@ export function isRootOpenaiBaseUrlLine(line: string): boolean { | |
| export function tomlStringPattern(key: string): RegExp { | ||
| const escaped = key.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); | ||
| const keyToken = `(?:${escaped}|"${escaped}"|'${escaped}')`; | ||
| return new RegExp(`^\\s*${keyToken}\\s*=\\s*["']([^"']+)["']\\s*(?:#.*)?$`); | ||
| // The quoted value is captured WITH its quotes so callers can decode it as TOML. | ||
| // A basic string escapes backslashes, so a Windows path is stored doubled; reading | ||
| // the raw bytes back returned a path that matched nothing on disk and made the | ||
| // journal's recorded catalog path un-restorable (#1798). | ||
| return new RegExp(`^\\s*${keyToken}\\s*=\\s*("(?:\\\\.|[^"])*"|'[^']*')\\s*(?:#.*)?$`); | ||
| } | ||
|
|
||
| export function rootTomlString(content: string, key: string): string | null { | ||
|
|
@@ -26,7 +32,7 @@ export function rootTomlString(content: string, key: string): string | null { | |
| const pattern = tomlStringPattern(key); | ||
| for (const line of rootLines) { | ||
| const match = pattern.exec(line); | ||
| if (match?.[1]) return match[1].trim(); | ||
| if (match?.[1]) return parseTomlString(match[1]).trim(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
bun -e 'import { parseTomlString } from "./src/codex/paths.ts"; const raw = "\"\\U0001F600\""; if (parseTomlString(raw) !== "😀") throw new Error("TOML Unicode escape was not decoded");'Repository: lidge-jun/opencodex Length of output: 198 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate files ---'
fd -t f '^(injected-marker|paths)\.ts$|package\.json$|bunfig\.toml$' .
printf '%s\n' '--- injected-marker.ts ---'
file="$(fd -t f '^injected-marker\.ts$' | head -n 1)"
cat -n "$file"
printf '%s\n' '--- paths.ts parseTomlString and nearby code ---'
paths="$(fd -t f '^paths\.ts$' | head -n 1)"
rg -n -A35 -B10 'parseTomlString|JSON\.parse' "$paths"
printf '%s\n' '--- parser usages and relevant tests ---'
rg -n -A8 -B8 'parseTomlString|injected-marker|injected marker|U0001F600' . --glob '!node_modules' --glob '!dist' --glob '!build'Repository: lidge-jun/opencodex Length of output: 50375 🏁 Script executed: #!/bin/bash
set -euo pipefail
node - <<'JS'
function parseTomlString(raw) {
if (raw.startsWith('"')) {
try {
return JSON.parse(raw);
} catch {
return raw.slice(1, -1);
}
}
return raw.slice(1, -1);
}
const raw = '"\\U0001F600"';
const decoded = parseTomlString(raw);
const expected = String.fromCodePoint(0x1f600);
console.log(JSON.stringify({ raw, decoded, expected, matches: decoded === expected }));
if (decoded === expected) process.exit(1);
JS
printf '%s\n' '--- package scripts ---'
node -e 'const p=require("./package.json"); console.log(JSON.stringify(p.scripts ?? {}, null, 2))'
printf '%s\n' '--- focused parser tests ---'
rg -n -g 'tests/**' -g 'src/**' 'parseTomlString|rootTomlString|providerTableString|stripJournaledOpenaiBaseUrl|hasInjectedCodexRouting' | head -n 120Repository: lidge-jun/opencodex Length of output: 1637 Use TOML-compatible escape decoding in
Decode the TOML basic-string escapes, including 🤖 Prompt for AI Agents |
||
| } | ||
| return null; | ||
| } | ||
|
|
@@ -45,7 +51,7 @@ export function providerTableString(content: string, provider: string, key: stri | |
| const pattern = tomlStringPattern(key); | ||
| for (let index = start + 1; index < lines.length && !/^\s*\[/.test(lines[index]); index += 1) { | ||
| const match = pattern.exec(lines[index]); | ||
| if (match?.[1]) return match[1].trim(); | ||
| if (match?.[1]) return parseTomlString(match[1]).trim(); | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,6 +39,31 @@ const SID_PATTERN = /^S-1-(?:\d+-)+\d+$/i; | |
| */ | ||
| const WINDOWS_POWERSHELL_LOOKUP_TIMEOUT_MS = 8_000; | ||
|
|
||
| /** | ||
| * FOLDERID_LocalAppData, and the flag that makes the lookup ignore the caller's | ||
| * environment. | ||
| * | ||
| * The obvious spelling, .NET's | ||
| * `GetFolderPath(SpecialFolder.LocalApplicationData)`, is unusable here: on | ||
| * Windows it resolves through `USERPROFILE`, and when the profile named there has | ||
| * no local AppData directory on disk it returns an EMPTY STRING rather than an | ||
| * error. Any | ||
| * caller that redirected `USERPROFILE` (the test sandbox does, and so does a | ||
| * service account whose profile has not been materialized) therefore refused every | ||
| * coordinator lookup with "returned an empty value", which is the exact | ||
| * environment dependence this module exists to eliminate. | ||
| * | ||
| * `SHGetKnownFolderPath` with a null token and `KF_FLAG_DEFAULT_PATH` reads the | ||
| * known-folder registration for the effective token instead: it returns the real | ||
| * per-user path whether or not the directory exists, and it is unaffected by | ||
| * `USERPROFILE`, `LOCALAPPDATA`, `HOMEDRIVE`, or `HOMEPATH`. A non-null token argument | ||
| * is NOT equivalent: passing (HANDLE)-1 resolves the DEFAULT USER profile | ||
| * (the built-in "Default" profile), which would key coordination to a namespace | ||
| * no real account writes to. | ||
| */ | ||
| const WINDOWS_LOCAL_APPDATA_FOLDER_ID = "F1B32785-6FBA-4FCF-9D55-7B8E7F157091"; | ||
| const WINDOWS_KF_FLAG_DEFAULT_PATH = "0x00000400"; | ||
|
|
||
| export class CodexUserIdentityRefusal extends Error { | ||
| readonly code = "CODEX_USER_IDENTITY_REFUSED"; | ||
|
|
||
|
|
@@ -93,6 +118,42 @@ export function windowsIdentityPowerShellCommandForTests(expression: string): st | |
| return windowsIdentityPowerShellCommand(expression); | ||
| } | ||
|
|
||
| /** | ||
| * PowerShell expression yielding the effective account's local AppData path. | ||
| * | ||
| * P/Invoke rather than a .NET convenience wrapper, for the reason recorded on | ||
| * WINDOWS_LOCAL_APPDATA_FOLDER_ID: the wrapper follows `USERPROFILE` and answers | ||
| * an empty string for a profile whose directory is absent, which is precisely | ||
| * the environment dependence this module refuses to inherit. The type is added | ||
| * under a unique name per process because `Add-Type` cannot redefine one. | ||
| * | ||
| * The whole sequence is wrapped in one `$(...)` subexpression because the caller | ||
| * substitutes this text into `[string](<expression>)`; several statements | ||
| * spliced in bare would close that cast's parenthesis early and fail to parse. | ||
| */ | ||
| function windowsLocalAppDataExpression(): string { | ||
| const signature = | ||
| '[DllImport("shell32.dll", CharSet = CharSet.Unicode)] public static extern int ' | ||
| + 'SHGetKnownFolderPath(ref System.Guid id, uint flags, System.IntPtr token, out System.IntPtr path);'; | ||
| const statements = [ | ||
| `$ocxShell = Add-Type -MemberDefinition '${signature}'` | ||
| + " -Name OcxKnownFolder -Namespace OcxIdentity -PassThru", | ||
| `$ocxFolderId = [System.Guid]'${WINDOWS_LOCAL_APPDATA_FOLDER_ID}'`, | ||
| "$ocxPathPtr = [System.IntPtr]::Zero", | ||
| "$ocxHr = $ocxShell::SHGetKnownFolderPath([ref]$ocxFolderId, " | ||
| + `${WINDOWS_KF_FLAG_DEFAULT_PATH}, [System.IntPtr]::Zero, [ref]$ocxPathPtr)`, | ||
| "if ($ocxHr -ne 0) { throw 'SHGetKnownFolderPath failed' }", | ||
| "try { [System.Runtime.InteropServices.Marshal]::PtrToStringUni($ocxPathPtr) }" | ||
| + " finally { [System.Runtime.InteropServices.Marshal]::FreeCoTaskMem($ocxPathPtr) }", | ||
| ]; | ||
| return `$(${statements.join("; ")})`; | ||
| } | ||
|
|
||
| /** Test-only readback of the environment-independent known-folder expression. */ | ||
| export function windowsLocalAppDataExpressionForTests(): string { | ||
| return windowsLocalAppDataExpression(); | ||
| } | ||
|
|
||
|
Comment on lines
+121
to
+156
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Add focused regression coverage for the changed lookup and parser contracts. Cover the known-folder expression and success-only caching in 📍 Affects 2 files
🤖 Prompt for AI AgentsSource: Path instructions |
||
| /** Test-only readback of the spawn options shared by the identity lookups (#1278). */ | ||
| export function windowsIdentityPowerShellSpawnOptionsForTests(): ReturnType< | ||
| typeof windowsIdentityPowerShellSpawnOptions | ||
|
|
@@ -122,7 +183,31 @@ export function decodeWindowsIdentityPowerShellOutputForTests(output: Uint8Array | |
| return decodeWindowsIdentityPowerShellOutput(output); | ||
| } | ||
|
|
||
| /** | ||
| * Per-process memo for the Windows lookups. | ||
| * | ||
| * Both values -- the effective token's SID and its known-folder local AppData -- | ||
| * are fixed for the lifetime of a process: neither can change without a new logon | ||
| * token, and the lookups deliberately ignore the environment, so nothing a caller | ||
| * does between two calls can alter the answer. Each call otherwise spawns a fresh | ||
| * PowerShell, roughly 150ms for the SID and 310ms for the folder, and the | ||
| * coordinator asks for both on every config write and lock acquisition. That cost | ||
| * pushed real multi-process injection tests past their budget while proving | ||
| * nothing: the second spawn re-derives what the first already established. | ||
| * | ||
| * Only successful lookups are memoized, so a transient failure cannot pin the | ||
| * process into a permanently refusing state. | ||
| */ | ||
| const windowsIdentityValueCache = new Map<string, string>(); | ||
|
|
||
| /** Test-only reset so a suite can force a fresh lookup. */ | ||
| export function resetWindowsIdentityValueCacheForTests(): void { | ||
| windowsIdentityValueCache.clear(); | ||
| } | ||
|
|
||
| function powershellValue(expression: string): string { | ||
| const memoized = windowsIdentityValueCache.get(expression); | ||
| if (memoized !== undefined) return memoized; | ||
| let command: string[]; | ||
| try { | ||
| command = windowsIdentityPowerShellCommand(expression); | ||
|
|
@@ -145,6 +230,7 @@ function powershellValue(expression: string): string { | |
| if (result.exitCode !== 0) refuse("Windows effective-account lookup failed."); | ||
| const value = decodeWindowsIdentityPowerShellOutput(result.stdout ?? Buffer.alloc(0)); | ||
| if (!value) refuse("Windows effective-account lookup returned an empty value."); | ||
| windowsIdentityValueCache.set(expression, value); | ||
| return value; | ||
| } | ||
|
|
||
|
|
@@ -265,9 +351,7 @@ export function probeCodexCoordinatorNamespace(identity: UserIdentity): Coordina | |
| } | ||
|
|
||
| if (!SID_PATTERN.test(identity.sid)) refuse("The coordinator identity contains an invalid SID."); | ||
| const localAppData = powershellValue( | ||
| "[Environment]::GetFolderPath([Environment+SpecialFolder]::LocalApplicationData)", | ||
| ); | ||
| const localAppData = powershellValue(windowsLocalAppDataExpression()); | ||
| if (!isAbsolute(localAppData)) refuse("Windows LocalAppData resolution returned a relative path."); | ||
| const root = resolve(localAppData, "OpenCodex", "Runtime", "v1", identity.sid.toUpperCase()); | ||
| let entry; | ||
|
|
@@ -297,9 +381,7 @@ export function probeCodexCoordinatorNamespace(identity: UserIdentity): Coordina | |
|
|
||
| function resolveWindowsRuntimeRoot(identity: Extract<UserIdentity, { platform: "win32" }>): string { | ||
| if (!SID_PATTERN.test(identity.sid)) refuse("The coordinator identity contains an invalid SID."); | ||
| const localAppData = powershellValue( | ||
| "[Environment]::GetFolderPath([Environment+SpecialFolder]::LocalApplicationData)", | ||
| ); | ||
| const localAppData = powershellValue(windowsLocalAppDataExpression()); | ||
| if (!isAbsolute(localAppData)) refuse("Windows LocalAppData resolution returned a relative path."); | ||
|
|
||
| // The SID and known-folder values come from the effective token/.NET OS APIs, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -122,57 +122,66 @@ export function rebuildLabProjection(configDir?: string): RebuildResult { | |
|
|
||
| const db = new Database(paths.sqlitePath); | ||
| let transactionOpen = false; | ||
| // Every statement prepared below, so they can be finalized before the close. | ||
| // | ||
| // Bun keeps a prepared statement alive until it is finalized or garbage collected, | ||
| // and on Windows an unfinalized statement holds the database file open: `close()` | ||
| // leaves the handle, and `close(true)` throws "database is locked". A second | ||
| // rebuild then failed to unlink the previous projection with EBUSY, and the retry | ||
| // loop in `wipeSqlite` could only turn that into a slower failure. POSIX allows | ||
| // unlinking an open file, which is why this never surfaced there. | ||
| const prepared: Array<{ finalize(): void }> = []; | ||
| const prepare = (sql: string) => { | ||
| const statement = db.prepare(sql); | ||
| prepared.push(statement); | ||
| return statement; | ||
| }; | ||
| try { | ||
| db.exec("PRAGMA journal_mode=DELETE;"); | ||
| db.exec("PRAGMA foreign_keys=OFF;"); | ||
| db.exec("BEGIN IMMEDIATE;"); | ||
| transactionOpen = true; | ||
| resetProjectionSchema(db); | ||
|
|
||
| db.prepare( | ||
| "INSERT OR REPLACE INTO schema_meta(key, value) VALUES (?, ?)", | ||
| ).run("schema_version", String(LAB_SQLITE_SCHEMA_VERSION)); | ||
| db.prepare( | ||
| "INSERT OR REPLACE INTO schema_meta(key, value) VALUES (?, ?)", | ||
| ).run("projection_spec_version", LAB_PROJECTION_SPEC_VERSION); | ||
| db.prepare( | ||
| "INSERT OR REPLACE INTO schema_meta(key, value) VALUES (?, ?)", | ||
| ).run("built_at_ms", String(Date.now())); | ||
| const insertMeta = prepare("INSERT OR REPLACE INTO schema_meta(key, value) VALUES (?, ?)"); | ||
| insertMeta.run("schema_version", String(LAB_SQLITE_SCHEMA_VERSION)); | ||
| insertMeta.run("projection_spec_version", LAB_PROJECTION_SPEC_VERSION); | ||
| insertMeta.run("built_at_ms", String(Date.now())); | ||
|
|
||
| const insertCorruption = db.prepare( | ||
| const insertCorruption = prepare( | ||
| "INSERT INTO corruption(kind, line_number, event_id, detail) VALUES (?, ?, ?, ?)", | ||
| ); | ||
| for (const c of corruptions) { | ||
| insertCorruption.run(c.kind, c.lineNumber ?? null, c.eventId ?? null, c.detail); | ||
| } | ||
|
|
||
| const insertEvent = db.prepare( | ||
| const insertEvent = prepare( | ||
| `INSERT INTO events(event_id, event_kind, recorded_at, producer, producer_version, payload_json, excluded, exclusion_reason) | ||
| VALUES (?, ?, ?, ?, ?, ?, ?, ?)`, | ||
| ); | ||
| const insertSubject = db.prepare( | ||
| const insertSubject = prepare( | ||
| `INSERT OR IGNORE INTO subjects(subject_id, subject_kind, subject_json) VALUES (?, ?, ?)`, | ||
| ); | ||
| const insertObs = db.prepare( | ||
| const insertObs = prepare( | ||
| `INSERT INTO observations( | ||
| event_id, subject_id, evidence_layer, suite_id, suite_version, suite_manifest_digest, | ||
| scenario_id, scenario_version, scenario_manifest_digest, outcome, completed_at, execution_mode | ||
| ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, | ||
| ); | ||
| const insertClaim = db.prepare( | ||
| const insertClaim = prepare( | ||
| `INSERT INTO claims( | ||
| event_id, subject_id, capability, polarity, source_manifest_digest, | ||
| effective_at, recorded_at, supersedes_json, current, usable | ||
| ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)`, | ||
| ); | ||
| const insertInv = db.prepare( | ||
| const insertInv = prepare( | ||
| `INSERT INTO invalidations(event_id, reason, targets_json, recorded_at, applied) VALUES (?, ?, ?, ?, ?)`, | ||
| ); | ||
| const insertPurge = db.prepare( | ||
| const insertPurge = prepare( | ||
| `INSERT INTO purges(event_id, target_event_ids_json, target_artifact_digests_json, purge_actions_json, recorded_at) | ||
| VALUES (?, ?, ?, ?, ?)`, | ||
| ); | ||
| const insertArtifact = db.prepare( | ||
| const insertArtifact = prepare( | ||
| `INSERT INTO artifacts(digest, artifact_class, media_type, byte_count, status, last_error) | ||
| VALUES (?, ?, ?, ?, ?, ?) | ||
| ON CONFLICT(digest) DO UPDATE SET | ||
|
|
@@ -319,7 +328,7 @@ export function rebuildLabProjection(configDir?: string): RebuildResult { | |
| } | ||
| } | ||
|
|
||
| const insertVerdict = db.prepare( | ||
| const insertVerdict = prepare( | ||
| `INSERT INTO verdicts( | ||
| projection_key, subject_id, evidence_layer, suite_id, suite_version, suite_manifest_digest, | ||
| projection_spec_version, verdict, as_of, scenario_manifest_digests_json, claim_source_digest, | ||
|
|
@@ -369,6 +378,15 @@ export function rebuildLabProjection(configDir?: string): RebuildResult { | |
| } catch { | ||
| // Closing the disposable DB is still safe if pragma restoration fails. | ||
| } | ||
| // Finalize before closing: an outstanding statement keeps the file open on | ||
| // Windows, and the next rebuild cannot unlink the projection it is replacing. | ||
| for (const statement of prepared) { | ||
| try { | ||
| statement.finalize(); | ||
| } catch { | ||
| // A statement already finalized by an error path is not a rebuild failure. | ||
| } | ||
| } | ||
|
Comment on lines
+381
to
+389
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift Add a Windows regression test for statement finalization. Line 381 implements the handle-release behavior that prevents the next rebuild from failing to replace the SQLite projection. This cohort changes Add a focused Windows test that calls As per path instructions: 🤖 Prompt for AI AgentsSource: Path instructions |
||
| db.close(); | ||
| artifactStore.close(); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: lidge-jun/opencodex
Length of output: 36322
🏁 Script executed:
Repository: lidge-jun/opencodex
Length of output: 1224
Make the basic-string matcher linear.
At
src/codex/injected-marker.ts:25,[^"]can also consume\. Malformed quoted values with many backslashes then cause excessive backtracking during parsing.Exclude backslashes from the second branch:
Proposed fix
📝 Committable suggestion
🧰 Tools
🪛 ast-grep (0.45.1)
[warning] 24-24: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
^\\s*${keyToken}\\s*=\\s*("(?:\\\\.|[^"])*"|'[^']*')\\s*(?:#.*)?$)Note: [CWE-1333] Inefficient Regular Expression Complexity
(regexp-from-variable)
🤖 Prompt for AI Agents
Source: Linters/SAST tools