Repository navigation
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. No ADR enforcement needed for PR #67473: it lacks the "implementation" label and has only 30 new lines in business logic directories (threshold: 100).
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
The descriptor-based read is the right direction, but the TLC evidence bundle is still mutable after the verdict is computed.
Blocking theme
.github/scripts/work-queue-formal-check.cjsnow classifies from the original descriptor, but it never re-materializesbundle/tlc.logfrom those trusted bytes. In the same path-replacement scenario covered by the new test, the uploaded log artifact can still disagree withresult.jsonandsummary.md.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 57 AIC · ⌖ 5.62 AIC · ⊞ 19.9K
Comment /review to run again
| log = fs.readFileSync(logPath, "utf8"); | ||
| log = readFileDescriptor(fd); | ||
| checkpoints = checkpointBundle(stateDir, bundleDir, options.checkpointMaxBytes ?? CHECKPOINT_MAX_BYTES, log.includes("Checkpointing completed"), env); | ||
| } finally { |
There was a problem hiding this comment.
Reading the verdict from the original file descriptor fixes the TOCTOU on classification, but bundle/tlc.log itself can still be swapped to attacker-controlled content, so the uploaded evidence no longer matches the result you just computed.
💡 Keep the artifact bound to the descriptor you trusted
The new replace_log test demonstrates this: after tlc.log is replaced with a symlink, readFileDescriptor(fd) sees the real TLC output, but every later consumer of bundle/tlc.log reads the replacement file instead. That means result.json / summary.md can say passed while the bundled tlc.log shows unrelated content.
Please either fail if logPath no longer resolves to the inode behind fd, or rewrite logPath from the descriptor content before packaging the bundle, for example:
const log = readFileDescriptor(fd);
fs.closeSync(fd);
fs.writeFileSync(logPath, log, { flag: "w", mode: 0o600 });That keeps the artifact and the computed verdict derived from the same bytes.
There was a problem hiding this comment.
🟡 Changes recommended
Descriptor reads and writes can truncate data, while concurrent ledger growth can bypass the configured size limit.
3 open findings
What changed in this PR
Hardens scanner-related YAML generation, file handling, and Markdown trace rendering.
Changes:
- Safely quotes maintenance-token YAML values.
- Uses exclusive/open-descriptor file operations.
- Escapes Markdown table cells and adds regression tests.
| File | Description |
|---|---|
specs/work-queue/verify_native_test.py |
Tests oversized ledger rejection. |
specs/work-queue/native_probe.cjs |
Reads ledgers through one descriptor. |
specs/work-queue/compare-evaluation.mjs |
Hardens evidence file handling. |
specs/eslint-factory/trace.test.mjs |
Tests Markdown escaping. |
specs/eslint-factory/trace.mjs |
Escapes every table cell. |
pkg/workflow/maintenance_workflow_yaml_jobs.go |
Quotes the maintenance token safely. |
pkg/workflow/maintenance_workflow_triggers_test.go |
Updates expected quoted YAML. |
pkg/workflow/maintenance_workflow_generation_fixes_test.go |
Tests special-character round-tripping. |
.github/scripts/work-queue-formal-check.test.cjs |
Tests path-replacement resistance. |
.github/scripts/work-queue-formal-check.cjs |
Uses exclusive files and descriptors. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| if (fs.fstatSync(fd).size > 80 * 1024 * 1024) throw new Error("resource_limit: adapter input exceeds 80 MiB"); | ||
| data = fs.readFileSync(fd, "utf8"); |
| function readFileDescriptor(fd) { | ||
| const { size } = fs.fstatSync(fd); | ||
| if (size === 0) return ""; | ||
| const buffer = Buffer.alloc(size); | ||
| const bytesRead = fs.readSync(fd, buffer, 0, size, 0); | ||
| return buffer.subarray(0, bytesRead).toString("utf8"); | ||
| } |
| const readFileDescriptor = fd => { | ||
| const { size } = fs.fstatSync(fd); | ||
| if (size === 0) return ""; | ||
| const buffer = Buffer.alloc(size); | ||
| const bytesRead = fs.readSync(fd, buffer, 0, size, 0); | ||
| return buffer.subarray(0, bytesRead).toString("utf8"); | ||
| }; |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /codebase-design (triage via pr-triage agent; classified bug_fix). This is a well-scoped hardening PR — each fix is paired with a regression test that reproduces the original scanner finding (YAML injection round-trip, symlink-swap log read, oversized-ledger-via-fd, Markdown cell escaping).
📋 Key Themes & Highlights
Key Themes
- Duplicated TOCTOU helper:
readFileDescriptorinspecs/work-queue/compare-evaluation.mjsis a byte-for-byte copy of the one added to.github/scripts/work-queue-formal-check.cjs. Exporting and reusing it would avoid future drift in a security-relevant code path (flagged inline). - One inline comment I posted about missing test coverage for the
resultFdcatch-path was based on a mistaken premise — I verified afterward (by running the script directly) that the existing"CLI checksum rejection retains actual and pinned digests..."test does exercise that exact path, sinceresult.jsonis written viawriteJSONbefore the checksum throw. No action needed there; please disregard that specific sub-claim if raised, I'm noting the correction here for transparency.
Positive Highlights
- ✅
writeYAMLEnvreuse for the maintenance token is the right fix — consistent with the existing scalar-escaping helper, andTestMaintenanceWorkflowQuotesCompileGitHubTokenround-trips the generated YAML throughyaml.v3.Unmarshalto prove the token survives intact, a strong regression test. - ✅
native_probe.cjsandwork-queue-formal-check.cjsnow checkfstatSync(fd).size/read from the already-open descriptor instead of re-opening the path — correctly closes the TOCTOU window the scanner flagged, andverify_native_test.py/replace_logtest cases exercise it. - ✅
readableTrace'sescapeCellcorrectly handles backslash-before-pipe ordering (escape\first, then|) and collapses all three newline variants to<br>, with a direct unit test (trace.test.mjs) asserting exact escaped output.
No blocking issues — approving with the one non-blocking suggestion (helper dedup) left as an inline comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 142.7 AIC · ⌖ 14.7 AIC · ⊞ 10.3K
Comment /matt to run again
| const jar = process.env.TLA2TOOLS_JAR; | ||
| assert(jar && path.isAbsolute(jar), "TLA2TOOLS_JAR must be an absolute pinned jar path"); | ||
| const hash = value => crypto.createHash("sha256").update(value).digest("hex"); | ||
| const readFileDescriptor = fd => { |
There was a problem hiding this comment.
[/codebase-design] readFileDescriptor is byte-for-byte identical to the one just added in .github/scripts/work-queue-formal-check.cjs. Export it from that module (it already exports several helpers) and import it here instead of duplicating the implementation.
💡 Suggested fix
// work-queue-formal-check.cjs
module.exports = { TLC_SHA256, classify, inventory, checkpointBundle, runVerification, readFileDescriptor };
// compare-evaluation.mjs
import { TLC_SHA256, classify, readFileDescriptor } from "../../.github/scripts/work-queue-formal-check.cjs";Two copies of a security-relevant TOCTOU-avoidance helper are easy to let drift out of sync during future edits.
@copilot please address this.
| } catch (openError) { | ||
| if (!openError || openError.code !== "ENOENT") throw openError; | ||
| } | ||
| if (resultFd !== undefined) { |
There was a problem hiding this comment.
[/diagnosing-bugs] This fd-based rewrite of the tool-error bundle is a good TOCTOU fix, but it's untested — the only exercised catch path (CLI checksum rejection retains actual and pinned digests..., line ~215 in the test file) fails before bundle/result.json exists, so it never opens the fd here. A regression test that lets writeJSON(resultPath, base) succeed, then forces runVerification to reject afterward (e.g. by making the spawned javaBin fixture exit with an unexpected signal) would confirm the read/rewrite path and guard the fd-leak-on-ENOENT edge against regressions.
💡 Why this matters
The prior version used fs.existsSync + fs.readFileSync/writeJSON (path-based, racy but simple). The new version opens with r+, swallows ENOENT, and closes in a finally — correct, but there's no test proving the merged JSON (status: "tool_error", preserved fields) is still written correctly through the descriptor, or that the fd is closed even when JSON.parse throws on corrupt content.
@copilot please address this.
Scanner findings covered unsafe generated YAML quoting, GraphQL interpolation, file races, trace escaping, and an unpinned Docker installer.
docker-sbxruntime.