fix(ops): growth/ops export scripts honor --help without overwriting tracked artifacts - #44
fix(ops): growth/ops export scripts honor --help without overwriting tracked artifacts#44nish3451 wants to merge 4 commits into
Conversation
…ting tracked artifacts Operator export scripts ignored --help and silently regenerated tracked ops artifacts (live-metrics.md, proof-library.md, sender-setup-guide, 11-10-proof-run, competitive-proof-matrix, market benchmark), destroying human-reviewed content whenever --help or -h was passed. They also accepted --output/--html/--ops/--loom-links paths that escape the repository and could write or overwrite files anywhere on the machine. - Add scripts/lib/operator-cli.mjs with handleHelp() (prints usage and exits 0 before any work) and resolveOutputPath() (refuses absolute, ..-traversal, and symlink escapes outside the service repository root). - Route every active export script's writes through resolveOutputPath and call handleHelp first: export-client-delivery-cockpit, export-daily-money-mission, export-followup-cockpit, export-growth-cockpit, export-growth-doctor, export-growth-metrics, export-internal-dashboard, export-lead-scoring-cockpit, export-managed-it-one-pager, export-market-benchmark, export-market-learning-review, export-market-proof-cockpit, export-market-proof-run, export-proof-library, export-prospect-outbox, export-recording-cockpit, export-recording-queue, export-recording-rehearsal-check, export-recording-teleprompter, export-sales-cockpit, export-sender-setup-guide. - export-internal-dashboard no longer writes its parity scratch file to /tmp; it uses a gitignored runs/ scratch path cleaned up after use. - Extend test-active-operator-surfaces: every active export script must exit 0 on --help, print usage, and leave live-metrics.md, the proof library, and the daily money mission untouched; escaping --output, --html, --ops, --loom-links and symlink paths are refused without creating the file.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
nish3451 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Lane item verification: Repro against fresh origin/main (a0d1de5): Against this branch (7b32739): both scripts print usage and exit 0 with the tracked artifacts untouched (sha1 verified); all 21 active export scripts honor |
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds shared CLI help handling and safe output-path resolution. Export scripts now use these helpers for generated files and Loom-link paths. Tests cover help behavior, artifact preservation, traversal, absolute paths, nested paths, and symlink escapes. ChangesOperator CLI safety
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/export-daily-money-mission.mjs`:
- Around line 11-13: Update resolveOutputPath in operator-cli.mjs to reject
absolute raw input paths before resolving or validating them against
serviceRoot. Preserve the existing relative-path resolution and validation
behavior for accepted inputs, including the callers such as handleHelp in the
daily mission script.
In `@scripts/export-followup-cockpit.mjs`:
- Around line 408-411: Replace manual slash-based parent-directory extraction
with the path module’s dirname() in the resolved output path flow, including the
matching loomLinksDir handling. Apply the same change to the corresponding path
handling in the listed export scripts, preserving recursive directory creation
before writeFileSync().
In `@scripts/export-internal-dashboard.mjs`:
- Around line 59-61: Update the parity scratch path handling around parity and
rmSync to call resolveOutputPath("runs/.internal-dashboard-parity.md") before
invoking runJson. Pass the resolved path in the --output argument and reuse that
same resolved value for cleanup, preserving the existing temporary-file removal
behavior.
In `@scripts/lib/operator-cli.mjs`:
- Around line 41-54: Close the check-to-use race in the path-validation helper
by replacing the `realpathSync(existing)` check followed by path-based
filesystem use with directory-handle-relative operations that keep validation
and creation/writing anchored to the trusted repository root. Ensure the later
`mkdirSync()` and `writeFileSync()` flows cannot follow a replaced ancestor
symlink, while preserving rejection through `refuse()` for paths escaping the
repository.
In `@scripts/test-active-operator-surfaces.mjs`:
- Around line 137-157: Update the escape probes in the test flow around
escapeProbes to use unique test-owned external paths instead of fixed /tmp
locations, avoiding interference with unrelated or parallel runs. Move cleanup
for all probe outputs and symlink fixtures into an unconditional finally block
so failures cannot leave artifacts or skip symlink cleanup.
- Around line 123-126: Update the help-surface test loop around run and the
existing Usage assertion to invoke every script with both --help and -h,
verifying each invocation exits successfully and prints usage.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 879b07ad-320e-4c3a-b006-5272345588f4
📒 Files selected for processing (23)
scripts/export-client-delivery-cockpit.mjsscripts/export-daily-money-mission.mjsscripts/export-followup-cockpit.mjsscripts/export-growth-cockpit.mjsscripts/export-growth-doctor.mjsscripts/export-growth-metrics.mjsscripts/export-internal-dashboard.mjsscripts/export-lead-scoring-cockpit.mjsscripts/export-managed-it-one-pager.mjsscripts/export-market-benchmark.mjsscripts/export-market-learning-review.mjsscripts/export-market-proof-cockpit.mjsscripts/export-market-proof-run.mjsscripts/export-proof-library.mjsscripts/export-prospect-outbox.mjsscripts/export-recording-cockpit.mjsscripts/export-recording-queue.mjsscripts/export-recording-rehearsal-check.mjsscripts/export-recording-teleprompter.mjsscripts/export-sales-cockpit.mjsscripts/export-sender-setup-guide.mjsscripts/lib/operator-cli.mjsscripts/test-active-operator-surfaces.mjs
| import { handleHelp, resolveOutputPath } from "./lib/operator-cli.mjs"; | ||
|
|
||
| handleHelp(process.argv.slice(2), `Usage: npm run growth:mission -- [--limit=5] [--output=runs/daily-money-mission.md] [--html=runs/daily-money-mission.html]`); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject absolute input paths in resolveOutputPath.
Line 13 relies on resolveOutputPath for the PR path policy. The shared implementation resolves raw before validation, so an absolute path inside serviceRoot passes validation. This does not meet the requirement to reject absolute paths.
Proposed fix in scripts/lib/operator-cli.mjs
export function resolveOutputPath(value, { flag = "--output", fallback } = {}) {
const raw = value === undefined || value === null || value === "" ? fallback : value;
if (typeof raw !== "string" || raw === "") {
refuse(flag, value ?? "", "an output path is required");
}
+ if (isAbsolute(raw)) {
+ refuse(flag, raw, "absolute paths are not allowed");
+ }
const root = resolve(serviceRoot);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/export-daily-money-mission.mjs` around lines 11 - 13, Update
resolveOutputPath in operator-cli.mjs to reject absolute raw input paths before
resolving or validating them against serviceRoot. Preserve the existing
relative-path resolution and validation behavior for accepted inputs, including
the callers such as handleHelp in the daily mission script.
| const resolvedOutputPath = resolveOutputPath(outputPath); | ||
| const outputDir = resolvedOutputPath.split("/").slice(0, -1).join("/"); | ||
| if (outputDir) mkdirSync(outputDir, { recursive: true }); | ||
| writeFileSync(outputPath, html); | ||
| writeFileSync(resolvedOutputPath, html); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -P 'const (outputDir|loomLinksDir) = .*resolved.*\.split\("/"\)\.slice\(0, -1\)\.join\("/"\)' scriptsRepository: nish3451/tinystudio-in
Length of output: 1616
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- path imports and helpers ---'
rg -n -C 3 'from "node:path"|from .path.|resolveOutputPath|resolveLoomLinksPath|outputDir|loomLinksDir' \
scripts/export-followup-cockpit.mjs \
scripts/export-growth-cockpit.mjs \
scripts/export-growth-doctor.mjs \
scripts/export-growth-metrics.mjs \
scripts/export-lead-scoring-cockpit.mjs \
scripts/export-market-proof-run.mjs \
scripts/export-proof-library.mjs \
scripts/export-prospect-outbox.mjs
printf '%s\n' '--- Node path behavior ---'
node - <<'JS'
const path = require('node:path');
const samples = [
'C:\\reports\\followup.html',
'\\\\server\\share\\reports\\followup.html',
'/reports/followup.html',
];
for (const sample of samples) {
console.log(JSON.stringify({
platform: process.platform,
sample,
posixSplit: sample.split('/').slice(0, -1).join('/'),
dirname: path.dirname(sample),
win32Dirname: path.win32.dirname(sample),
posixDirname: path.posix.dirname(sample),
}));
}
JSRepository: nish3451/tinystudio-in
Length of output: 15671
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- resolveOutputPath implementation ---'
rg -n -C 8 'function resolveOutputPath|const resolveOutputPath|export .*resolveOutputPath|resolve\(' scripts/lib/operator-cli.mjs
printf '%s\n' '--- every matching separator-based directory derivation ---'
rg -n -C 2 'split\("/"\)\.slice\(0, -1\)\.join\("/"\)' scripts
printf '%s\n' '--- existing dirname usage and path imports ---'
rg -n 'dirname|from "node:path"' scripts -g '*.mjs'Repository: nish3451/tinystudio-in
Length of output: 36660
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const path = require("node:path");
const root = "C:\\work\\tinystudio-in";
const raw = "prospects\\followup-cockpit.html";
const resolved = path.win32.resolve(root, raw);
const splitDir = resolved.split("/").slice(0, -1).join("/");
const dirnameDir = path.win32.dirname(resolved);
console.log(JSON.stringify({ resolved, splitDir, dirnameDir }));
JS
python3 - <<'PY'
from pathlib import Path
import re
for file in sorted(Path("scripts").glob("*.mjs")):
text = file.read_text()
for match in re.finditer(r'^const (outputDir|loomLinksDir|dir) = (.+split\("/"\).slice\(0, -1\).join\("/"\));$', text, re.M):
expr = match.group(2)
if "resolved" in expr or file.name in {
"export-recording-queue.mjs",
"export-recording-cockpit.mjs",
"export-sales-cockpit.mjs",
"export-recording-teleprompter.mjs",
"export-daily-money-mission.mjs",
}:
line = text[:match.start()].count("\n") + 1
print(f"{file}:{line}: {match.group(1)} = {expr}")
PYRepository: nish3451/tinystudio-in
Length of output: 1675
Use dirname() for resolved paths.
On Windows, splitting a resolved path on / can pass the complete filename to mkdirSync(). The subsequent writeFileSync() then fails with EISDIR.
Use dirname() for all matching paths, including loomLinksDir, and update the additional matches in scripts/export-recording-queue.mjs, scripts/export-recording-cockpit.mjs, scripts/export-sales-cockpit.mjs, scripts/export-recording-teleprompter.mjs, and scripts/export-daily-money-mission.mjs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/export-followup-cockpit.mjs` around lines 408 - 411, Replace manual
slash-based parent-directory extraction with the path module’s dirname() in the
resolved output path flow, including the matching loomLinksDir handling. Apply
the same change to the corresponding path handling in the listed export scripts,
preserving recursive directory creation before writeFileSync().
| const parityScratchPath = "runs/.internal-dashboard-parity.md"; | ||
| const parity = runJson(["scripts/check-market-parity-readiness.mjs", "--skip-kit", `--output=${parityScratchPath}`]); | ||
| try { rmSync(join(serviceRoot, parityScratchPath), { force: true }); } catch {} |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline scripts/check-market-parity-readiness.mjs --items all
rg -n -C 5 -- '--output|resolveOutputPath|writeFileSync|mkdirSync' \
scripts/check-market-parity-readiness.mjsRepository: nish3451/tinystudio-in
Length of output: 4750
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate files ---'
fd -t f 'export-internal-dashboard\.mjs|runtime-roots\.mjs' scripts
printf '%s\n' '--- exporter outline ---'
ast-grep outline scripts/export-internal-dashboard.mjs --items all
printf '%s\n' '--- resolver and parity call sites ---'
rg -n -C 8 'resolveOutputPath|parityScratchPath|check-market-parity-readiness|rmSync|serviceRoot|runs/' \
scripts/export-internal-dashboard.mjs scripts/lib/runtime-roots.mjs scripts
printf '%s\n' '--- relevant exporter source ---'
sed -n '1,130p' scripts/export-internal-dashboard.mjs
printf '%s\n' '--- runtime roots source ---'
sed -n '1,220p' scripts/lib/runtime-roots.mjsRepository: nish3451/tinystudio-in
Length of output: 50379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
log=/tmp/coderabbit-shell-logs/shell-output-g3CrRC
printf '%s\n' '--- exporter markers in saved output ---'
rg -n -A 90 -B 10 '^scripts/export-internal-dashboard\.mjs' "$log" | head -n 180
printf '%s\n' '--- resolver definitions and imports ---'
rg -n -A 35 -B 8 'function resolveOutputPath|const resolveOutputPath|export .*resolveOutputPath|operator-cli\.mjs' \
scripts/lib/operator-cli.mjs scripts/export-internal-dashboard.mjs
printf '%s\n' '--- exact relevant exporter lines ---'
nl -ba scripts/export-internal-dashboard.mjs | sed -n '1,90p'
printf '%s\n' '--- exact resolver lines ---'
nl -ba scripts/lib/operator-cli.mjs | sed -n '1,180p'Repository: nish3451/tinystudio-in
Length of output: 20531
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
exporter = Path("scripts/export-internal-dashboard.mjs").read_text()
child = Path("scripts/check-market-parity-readiness.mjs").read_text()
resolver = Path("scripts/lib/operator-cli.mjs").read_text()
checks = {
"exporter imports resolver": 'import { handleHelp, resolveOutputPath } from "./lib/operator-cli.mjs";' in exporter,
"raw scratch path passed to child": re.search(
r'runJson\(\["scripts/check-market-parity-readiness\.mjs", "--skip-kit", `--output=\$\{parityScratchPath\}`\]\)',
exporter,
) is not None,
"raw scratch path used for cleanup": re.search(
r'rmSync\(join\(serviceRoot, parityScratchPath\), \{ force: true \}\)',
exporter,
) is not None,
"resolver checks lexical escape": 'relative(root, resolved)' in resolver and 'the path escapes the repository' in resolver,
"resolver checks symlink escape": 'realpathSync(existing)' in resolver and 'the path escapes the repository through a symlink' in resolver,
"child preserves absolute output": re.search(
r'const resolvedOutputPath = isAbsolute\(outputPath\) \? outputPath : join\(serviceRoot, outputPath\);',
child,
) is not None,
}
for name, result in checks.items():
print(f"{name}: {'PASS' if result else 'FAIL'}")
if not all(checks.values()):
raise SystemExit(1)
PYRepository: nish3451/tinystudio-in
Length of output: 383
Resolve the parity scratch path before use. scripts/check-market-parity-readiness.mjs accepts an absolute --output path, so pass resolveOutputPath("runs/.internal-dashboard-parity.md") to the child and use that same resolved path in rmSync. This prevents a symlinked runs directory from redirecting the write or cleanup outside serviceRoot.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/export-internal-dashboard.mjs` around lines 59 - 61, Update the
parity scratch path handling around parity and rmSync to call
resolveOutputPath("runs/.internal-dashboard-parity.md") before invoking runJson.
Pass the resolved path in the --output argument and reuse that same resolved
value for cleanup, preserving the existing temporary-file removal behavior.
| // Symlink-escape guard: the nearest existing ancestor must stay inside the | ||
| // real repository root. | ||
| let existing = resolved; | ||
| while (!existsSync(existing)) { | ||
| const parent = dirname(existing); | ||
| if (parent === existing) break; | ||
| existing = parent; | ||
| } | ||
| const realRoot = realpathSync(root); | ||
| const realRel = relative(realRoot, realpathSync(existing)); | ||
| if (realRel === ".." || realRel.startsWith(`..${sep}`) || isAbsolute(realRel)) { | ||
| refuse(flag, raw, "the path escapes the repository through a symlink"); | ||
| } | ||
| return resolved; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Close the symlink check-to-use race.
If another principal can modify a directory below serviceRoot, it can replace the checked ancestor with an external symlink after realpathSync(existing) returns. The later mkdirSync() or writeFileSync() then follows that symlink and writes outside the repository.
Use directory-handle-relative filesystem operations, or enforce trusted ownership and non-writable permissions for all output ancestors before this helper is used.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/lib/operator-cli.mjs` around lines 41 - 54, Close the check-to-use
race in the path-validation helper by replacing the `realpathSync(existing)`
check followed by path-based filesystem use with directory-handle-relative
operations that keep validation and creation/writing anchored to the trusted
repository root. Ensure the later `mkdirSync()` and `writeFileSync()` flows
cannot follow a replaced ancestor symlink, while preserving rejection through
`refuse()` for paths escaping the repository.
| const helped = run([`scripts/${name}`, "--help"]) | ||
| eq(helped.status, 0, `${name} --help must exit 0: ${helped.stderr || helped.stdout}`) | ||
| mat(helped.stdout, /Usage:/, `${name} --help must print usage`) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Cover the -h help alias.
The PR contract requires both --help and -h. This loop invokes only --help, so a script can regress on -h while this suite passes. Run every help surface with both flags.
Proposed test update
- for (const name of helpSurface) {
- const helped = run([`scripts/${name}`, "--help"])
- eq(helped.status, 0, `${name} --help must exit 0: ${helped.stderr || helped.stdout}`)
- mat(helped.stdout, /Usage:/, `${name} --help must print usage`)
+ for (const flag of ["--help", "-h"]) {
+ for (const name of helpSurface) {
+ const helped = run([`scripts/${name}`, flag])
+ eq(helped.status, 0, `${name} ${flag} must exit 0: ${helped.stderr || helped.stdout}`)
+ mat(helped.stdout, /Usage:/, `${name} ${flag} must print usage`)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const helped = run([`scripts/${name}`, "--help"]) | |
| eq(helped.status, 0, `${name} --help must exit 0: ${helped.stderr || helped.stdout}`) | |
| mat(helped.stdout, /Usage:/, `${name} --help must print usage`) | |
| } | |
| for (const flag of ["--help", "-h"]) { | |
| for (const name of helpSurface) { | |
| const helped = run([`scripts/${name}`, flag]) | |
| eq(helped.status, 0, `${name} ${flag} must exit 0: ${helped.stderr || helped.stdout}`) | |
| mat(helped.stdout, /Usage:/, `${name} ${flag} must print usage`) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/test-active-operator-surfaces.mjs` around lines 123 - 126, Update the
help-surface test loop around run and the existing Usage assertion to invoke
every script with both --help and -h, verifying each invocation exits
successfully and prints usage.
| ["scripts/export-growth-metrics.mjs", ["--output=../escape-metrics.md"], join(T, "../escape-metrics.md")], | ||
| ["scripts/export-growth-cockpit.mjs", ["--output=/tmp/escape-cockpit.html"], "/tmp/escape-cockpit.html"], | ||
| ["scripts/export-daily-money-mission.mjs", ["--html=../../escape-mission.html"], join(T, "../../escape-mission.html")], | ||
| ["scripts/export-market-proof-run.mjs", ["--output=runs/escape-run.md", "--loom-links=../escape-links.txt"], join(T, "../escape-links.txt")], | ||
| ["scripts/export-market-benchmark.mjs", ["--ops=/tmp/escape-ops.md"], "/tmp/escape-ops.md"] | ||
| ] | ||
| for (const [script, args, escapePath] of escapeProbes) { | ||
| rmSync(escapePath, {force: true}) | ||
| const refused = run([script, ...args]) | ||
| neq(refused.status, 0, `${script} must refuse an escaping output path`) | ||
| eq(existsSync(escapePath), false, `${script} must not create ${escapePath}`) | ||
| } | ||
| const outsideLink = join(T, "..", "escape-symlink-outside") | ||
| mkdirSync(outsideLink, {recursive: true}) | ||
| const escapeSymlink = join(T, "escape-symlink") | ||
| symlinkSync(outsideLink, escapeSymlink) | ||
| const symlinkProbe = run(["scripts/export-growth-metrics.mjs", `--output=${escapeSymlink}/metrics.md`]) | ||
| neq(symlinkProbe.status, 0, "export must refuse an output path escaping through a symlink") | ||
| eq(existsSync(join(outsideLink, "metrics.md")), false, "symlink escape must not create the file outside the root") | ||
| rmSync(outsideLink, {recursive: true, force: true}) | ||
| unlinkSync(escapeSymlink) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use test-owned external paths and unconditional cleanup.
Lines 138 and 141 use fixed paths under /tmp. Line 144 deletes those paths before each probe. This can delete unrelated local files and cause parallel test interference. Also, a failed assertion skips the symlink cleanup at Lines 156-157.
Create unique test-owned external paths and clean all probe fixtures in a finally block.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/test-active-operator-surfaces.mjs` around lines 137 - 157, Update the
escape probes in the test flow around escapeProbes to use unique test-owned
external paths instead of fixed /tmp locations, avoiding interference with
unrelated or parallel runs. Move cleanup for all probe outputs and symlink
fixtures into an unconditional finally block so failures cannot leave artifacts
or skip symlink cleanup.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b327394ee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| write(resolveOutputPath(outputPath), markdown); | ||
| write(resolveOutputPath(opsPath, { flag: "--ops" }), markdown); | ||
| write(resolveOutputPath(htmlPath, { flag: "--html" }), html); |
There was a problem hiding this comment.
When market:benchmark receives a valid/default --output but an escaping --ops or --html, line 348 overwrites the tracked benchmark before the later path is validated and rejected. The command therefore exits with an error only after mutating an artifact—the behavior this safety change is intended to prevent—so resolve every destination immediately after argument parsing and before running generators or performing any writes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
nish3451 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef9d646e51
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let existing = resolved; | ||
| while (!existsSync(existing)) { | ||
| const parent = dirname(existing); | ||
| if (parent === existing) break; | ||
| existing = parent; |
There was a problem hiding this comment.
When an in-repository output is a dangling symlink to an outside file whose parent exists, existsSync(existing) is false, so this loop skips the symlink and validates only its in-repository parent. The exporter then follows the symlink during writeFileSync and creates or overwrites the external target; for example, a repository symlink report.md -> /tmp/outside.md makes --output=report.md pass validation and write /tmp/outside.md. Use symlink-aware inspection such as lstatSync while walking ancestors so dangling links are also rejected.
Useful? React with 👍 / 👎.
| `; | ||
|
|
||
| const outputDir = outputPath.split("/").slice(0, -1).join("/"); | ||
| const resolvedOutputPath = resolveOutputPath(outputPath); |
There was a problem hiding this comment.
When growth:cockpit receives an escaping --output, validation does not happen until this line, after lines 32–43 have already run the recording, mission, metrics, proof-library, doctor, and sender-guide exporters. The command therefore exits with a refusal only after overwriting tracked artifacts such as live-metrics.md and proof-library.md; resolve the requested path immediately after parsing arguments, before invoking any nested exporter.
Useful? React with 👍 / 👎.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
nish3451 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
The fleet keeps dispatching the same finding to multiple lanes, producing duplicate same-fix PR pairs: #36/#44 and #55/#56 are byte-identical or near-identical patches of the operator export --help fix, #39/#49 the offername article fix, #40/#52 the recording exporter --help fix, plus #38/#48/#51, #30/#61/#72, #45/#64, #42/#60, #43/#54 and #46/#74. Add a PR Duplicate Guard: - scripts/check-pr-duplicates.mjs compares a PR's diff against every other open PR (shared changed-file coverage >= 0.8 and patch similarity >= 0.5). Calibrated on all 79 open PRs on 2026-08-11: every pair above the thresholds was a genuine duplicate-cluster member, zero false positives. - .github/workflows/pr-duplicate-guard.yml runs it on every PR event and posts one marker comment naming the duplicate(s) and the canonical PR; the check fails loudly when a duplicate is found. Informational, not required. - scripts/test-pr-duplicates.mjs covers parsing, similarity, detection, and comment upsert with an injected API; wired into npm ci and npm test.
# Conflicts: # scripts/export-market-proof-run.mjs
There was a problem hiding this comment.
nish3451 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2112407ba5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ["scripts/export-market-benchmark.mjs", ["--ops=/tmp/escape-ops.md"], "/tmp/escape-ops.md"] | ||
| ] | ||
| for (const [script, args, escapePath] of escapeProbes) { | ||
| rmSync(escapePath, {force: true}) |
There was a problem hiding this comment.
When these predictable external paths already belong to a developer, another test, or a concurrent CI job, this unconditional cleanup deletes them before testing the refusal. The probe table includes /tmp/escape-cockpit.html, /tmp/escape-ops.md, and paths that normalize as far as /escape-mission.html; the later symlink probe similarly removes the shared /tmp/escape-symlink-outside directory recursively. Build all escape targets beneath a separate uniquely created temporary directory so the test never removes unrelated filesystem contents.
Useful? React with 👍 / 👎.
|
Correction to my closing comment above: the keeper for this cluster is #160, not #56. I had wrongly concluded that This PR stays closed either way — its content is superseded by what is already on |
…main by PR #80 (duplicate guard) (#234) The duplicate-PR guard (PR #80, merged 2026-08-19) is now live on main, preventing the same-fix duplication pattern at the CI level. The named duplicate pairs (#36/#44, #39/#49, #40/#52) are functionally superseded: their underlying fixes are on main via PRs #135, #145, and #178. The prior 2026-08-15 lane run had concluded the guard existed but was stuck unmerged; on 2026-08-19 it landed and the item is resolved at the root-cause level. Verification-only run (no product or test files touched): - PR #80 source commit 2091c7a and merge commit 0a9909b are ancestors of origin/main - scripts/test-pr-duplicates.mjs → 'test-pr-duplicates: ok' (exit 0) - Superseding PRs #135 (d4f3ef4), #145 (fc44b42), #178 (77f6922) all on main - Guard workflow runs on pull_request events; not a required status, so existing work never blocks Co-authored-by: minimax-vps <minimax-vps@nish3451.dev>
1 participant
What
Closes the lane item:
[unreviewed-by-grok] Growth/ops exporters ignore --help and overwrite tracked ops artifacts (live-metrics, proof-library).Operator export scripts ignored
--help/-hand silently regenerated tracked ops artifacts (growth-brain/ops/live-metrics.md,growth-brain/ops/proof-library.md,sender-setup-guide,11-10-proof-run,competitive-proof-matrix,market-parity-benchmark-2026), destroying human-reviewed content on every help invocation. They also accepted--output/--html/--ops/--loom-linkspaths that escape the repository and could write or overwrite files anywhere on the machine.Changes
scripts/lib/operator-cli.mjshandleHelp(args, usage)— prints usage and exits 0 before any work when--help/-his present.resolveOutputPath(value, {flag, fallback})— resolves operator-supplied output paths against the service repository root and refuses paths that escape it via absolute paths,..traversal, or symlink escapes (clear error, exit 1, no file created).handleHelpfirst and route every write (and the proof-run loom-links read) throughresolveOutputPath.export-internal-dashboard.mjsno longer writes its parity scratch file to/tmp; it uses a gitignoredruns/scratch path cleaned up after use.test-active-operator-surfaces.mjsextended:--help, print usage, and leavelive-metrics.md, the proof library, and the daily money mission untouched;--output=../…, absolute/tmp/…,../../html path,--loom-links=../…,--ops=/tmp/…, and an in-repo symlink escape are all refused without creating the file.Retired export scripts (no npm wiring, e.g.
export-owned-*,export-full-stack-growth-map) are not part of the active operator surface and are intentionally untouched.Validation
origin/main:node scripts/export-growth-metrics.mjs --helpandnode scripts/export-proof-library.mjs --helppreviously exited 0 while regenerating the tracked artifacts; on this branch both print usage, exit 0, and leave the tracked files byte-identical (sha1 verified).npm run ci: every suite passes (test-sales-intake-contract,test-active-offer-projection,test-active-operator-surfaces,test-direction-proof-gate,test-client-readiness-contract,test-validated-service-client,test-client-acceptance-gates,check-product-truth,check-human-service-kit,test-design-system-proving-lab, retention/agency-defaults/claims/send-readiness checks, all public-page suites, andnode --checkon every script).test-service-engine.mjs("round N: 2 simultaneous holders") is a pre-existing lock-timing flake — it fails identically on pristineorigin/main(2/4 runs in a scratch worktree) and passes on re-run; this branch touches none of that suite's code.Relationship to existing PR
This is a fresh implementation of the same lane item on current
main. PR #36 (fix/operator-export-cli-help, identical scope, same final file content) is the prior attempt from an earlier lane. Merge one and close the other — no manual conflict resolution needed.Files changed
scripts/lib/operator-cli.mjs(new)scripts/scripts/test-active-operator-surfaces.mjsSummary by CodeRabbit
New Features
--helpand-hguidance across export commands.Bug Fixes
Tests