fix(core): accept a host-supplied Maestro scope root

[pranavz28]

Problem

Maestro screenshots taken on BrowserStack App Automate 404 at the relay, so the Percy
build finalizes with zero snapshots and fails with no_snapshots:

[percy] Uploading: components-fundingform-checklist--default
[percy] Upload failed: 404
Screenshot not found: components-fundingform-checklist--default.png
  (searched /tmp/<sessionId>/*_maestro_debug_*/**/<name>.png)

App Automate has relocated its per-session artifacts to a per-device tree. On iOS the
Maestro debug output now lands at:

/usr/local/.browserstack/app-automate-tmp/<device>/logs/maestro_debug_<flow>/

The relay composes its BrowserStack search path as {tmpRoot}/{sessionId}{_test_suite},
where tmpRoot comes from PERCY_APP_AUTOMATE_TMP_DIR. That composition cannot reach the
new layout, for two independent reasons:

1. The new path has no <sessionId> segment — the CLI always appends one, and the
   host cannot absorb <device>/logs into a tmp-root value that the CLI will then suffix.
2. The iOS glob keys on a <device>_ prefix (*_maestro_debug_*) that the new
   directory names no longer carry.

Android is unaffected: its relocation preserved the {root}/{sessionId}_test_suite/logs/...
shape below the new root, so relocating the root composes correctly there.

Change

Adds PERCY_MAESTRO_BS_SCOPE_ROOT. When the host sets it, that value is the scope root:

• the relay globs {root}/**/{name}.png with no layout assumption;
• the manual-walker fallback recurses the same root;
• the realpath containment check anchors on it.

Decisions

A new variable rather than reusing PERCY_APP_AUTOMATE_TMP_DIR. That variable is
defined as a root the CLI composes onto. Overloading it to sometimes mean "the complete
root" would make its meaning depend on platform and on whether the composed path happens to
exist. The two variables answer different questions — "where does the tree start" vs. "what
exactly is the tree" — so they stay separate. PERCY_APP_AUTOMATE_TMP_DIR keeps working
unchanged for the Android layout and for any future relocation that only moves the root.

The override wins over the composed convention. If a host sets both, the explicit root
is the more specific statement of intent. Covered by a test.

The security boundary is relocated, never widened. The realpath + separator-prefix
containment check is unchanged; it just anchors on the overridden root. A filePath supplied
by the SDK is still rejected unless it resolves inside that root, and a symlink placed inside
the root that points outside it is still rejected. Both are covered by tests.

Non-absolute values are ignored (fall back to the composed convention). A malformed
injection should degrade to today's behaviour, not produce a cwd-relative root. This also
disposes of '/': trailing separators are trimmed first, so '/' becomes '', which is not
absolute and is therefore ignored — the whole filesystem can never become the scope root.

Trailing separators are trimmed so the glob pattern and the containment prefix compose
cleanly whether or not the host includes one.

No existence pre-check on the override. The self-hosted path 400s on a missing
PERCY_MAESTRO_SCREENSHOT_DIR because that is customer configuration and the customer can
fix it. This root is host configuration; a stale value is not something the customer can
act on, so it 404s like any other missing screenshot rather than surfacing a 400 that reads
as user error.

recursiveScope is derived inside the handler, not accepted from the request. It is
computed from selfHosted and the env var and passed down as a separate flag, so
selfHosted keeps its existing meaning everywhere it is already used (dot: true on the
glob, the walker fallback returning [], and the wording of the "resolved outside" error).

The override applies to Android too. The root replaces the entire convention, not just
the iOS arm — nothing about the mechanism is platform-specific, and a future Android
relocation of the same shape should not need a second change here.

The manual walker mirrors the glob. Its accept predicate keeps the _maestro_debug_
guard only on the conventional iOS path; with an explicit root the host has already narrowed
the tree to one session, so there is no convention left to filter on and any exact
{name}.png match is taken.

The recursive glob is bounded and matches the walker. {root}/**/{name}.png previously
had no depth limit while manualScreenshotWalk capped at 15, so the primary lookup and its
fallback disagreed about how far they would descend. Both now share one MAX_SEARCH_DEPTH
constant. This is a small behaviour change for existing self-hosted users with a
--test-output-dir tree deeper than 15 levels; that depth is implausible for Maestro output,
and the alternative is leaving an unbounded traversal on a root chosen by host config.

dot: true is keyed on the search pattern, not on selfHosted. It was previously enabled
only for self-hosted, on the reasoning that BrowserStack layouts contain no dot-prefixed
segments. That reasoning does not survive this change: once an operator can point the root
anywhere, a dot-prefixed segment beneath it is possible, and fast-glob would skip what the
walker fallback would happily find. Both lookup paths now agree.

The containment-failure message names the variable actually in play. It previously read
session dir for anything that was not self-hosted, which under the override would point an
operator at the wrong knob. All three roots now name themselves.

Session scoping under the override

sessionId still narrows the search when the injected root allows it. If
{root}/{sessionId} is a directory, that subtree becomes the scope root — so the glob, the
walker fallback and the realpath containment check all narrow to a single session, and a
filePath pointing into a sibling session's subtree is rejected. Roots that contain no such
subdirectory are used as given, which is the case for the per-device layout this change
exists for: it has no sessionId segment for the CLI to key on, so there is nothing to
narrow to.

That leaves one residual case — a root shared by concurrent sessions that does not carry
per-session subdirectories. The precondition there is explicit: the root a host injects must
contain only what the CLI process serving that session may read. On App Automate this holds
— the host starts the CLI per session, and the root it supplies belongs to a device allocated
exclusively to that session for its duration. A host that cannot guarantee that should inject
either a per-session root or a root containing per-session subdirectories; both work.

Where the same name genuinely does appear more than once under one root, the pre-existing
rule applies unchanged: exact filename match, newest mtime wins. Both behaviours are now
pinned by specs — one where a sibling session holds a newer file of the same name and the
narrowed lookup still returns the caller's own, and one where two session subtrees share a
root that is not narrowed.

Backwards compatibility

With PERCY_MAESTRO_BS_SCOPE_ROOT unset, every code path is byte-identical to before:
BrowserStack Android, BrowserStack iOS, the filePath fast path, and self-hosted
(PERCY_MAESTRO_SCREENSHOT_DIR) all behave exactly as they do today. Old CLIs simply ignore
the variable.

Rollout note

This is the CLI half. It only takes effect once the App Automate host injects
PERCY_MAESTRO_BS_SCOPE_ROOT and pins a CLI release that contains this change — a host
that injects the variable while pinned to a release without it falls back silently to the
legacy path and the 404 persists.

Tests

Fifteen specs added under /percy/maestro-screenshot, covering: finding a screenshot the
platform convention cannot reach; the same on Android; precedence over
PERCY_APP_AUTOMATE_TMP_DIR; trailing-slash tolerance; non-absolute values falling back;
filePath containment re-anchored on the overridden root (in-root resolves, out-of-root
rejected); a symlink inside the root that escapes it being rejected; a 404 for a
non-existent root; the trim/null semantics of the exported helper; a dot-prefixed directory
beneath the override root; a name shared by two session subtrees resolving to the newest
file; and a containment escape on the composed BrowserStack path, which had no coverage
before; narrowing to a session subtree when the root has one, including a sibling session
holding a newer same-named file and a filePath aimed at another session's subtree; and a
session-named entry that is not a directory falling back to the root.

Verified locally: the full /percy/maestro-screenshot suite (80 specs — 65 pre-existing plus
the 15 new) passes, and eslint is clean on all three changed files.

[pranavz28]

Claude Code Review (automated) — 2 inline finding(s). Full report in the PR comment below. Verdict: Failed - see PR comment.

[pranavz28]

Claude Code PR Review

PR: #2404 • Head: 862606e • Reviewers: stack-code-reviewer

Summary

Adds PERCY_MAESTRO_BS_SCOPE_ROOT so a BrowserStack App Automate host can supply the Maestro screenshot scope root directly, instead of the CLI composing {tmpRoot}/{sessionId}[_test_suite] — which cannot reach App Automate's new per-device .../<device>/logs/maestro_debug_<flow>/ layout. When set, the relay globs {root}/**/{name}.png, the manual-walker fallback recurses the same root, and the realpath containment check anchors on it. With the variable unset every path is byte-identical to before.

The mechanics are well built: the realpath + separator-prefix containment check is preserved on both the filePath and glob branches, bsScopeRootOverride() handles trailing separators / non-absolute / bare-/ safely ('/' trims to '', fails isAbsolute, so the filesystem root can never become the scope), and the manualScreenshotWalk refactor keeps the _maestro_debug_ filter on the conventional iOS arm while sharing one walker. Comments are unusually good and the PR body reasons explicitly about most of the design space.

The blocking issue is what the override removes rather than what it adds: sessionId no longer participates in scoping, and this relay demonstrably serves multiple sessions per process.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	Only env-var reads; no secrets introduced.
High	Security	Authentication/authorization checks present	N/A	Local relay endpoint; this PR changes no auth surface.
High	Security	Input validation and sanitization	Pass	name/sessionId SAFE_ID-validated; filePath shape-checked (string, absolute, ≤1024); override must be absolute after trailing-separator trim.
High	Security	No IDOR — resource ownership validated	Fail	Finding 1 — under the override, containment proves only "under the shared root", not "belongs to this session".
High	Security	No SQL injection (parameterized queries)	N/A	No SQL in this change.
High	Correctness	Logic is correct, handles edge cases	Fail	Finding 1 — same-name screenshots from concurrent sessions under one root resolve by newest-mtime, silently returning another session's image.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Errors are explicit ServerErrors; the catch {} around realpath deliberately 404s. Label wording is off under the override (Finding 3).
High	Correctness	No race conditions or concurrency issues	Fail	Finding 1 — the failure is specifically concurrent sessions sharing one process-level root.
Medium	Testing	New code has corresponding tests	Pass	9 new specs covering trim, non-absolute fallback, precedence, filePath re-anchoring, symlink escape, missing root, helper semantics.
Medium	Testing	Error paths and edge cases tested	Pass	Good negative coverage; the one untested case is the cross-session collision in Finding 1.
Medium	Testing	Existing tests still pass (no regressions)	Pass	CI green: 46 checks passing. Test @percy/core passed on one matrix leg (20m17s); the second leg was still running at review time.
Medium	Performance	No N+1 queries or unbounded data fetching	Fail	Finding 4 — {root}/**/{name}.png has no depth bound while the manual walker caps at 15. Low severity, pre-existing pattern now extended to BS traffic.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable to this request path.
Medium	Quality	Follows existing codebase patterns	Pass	Mirrors the existing appAutomateTmpDir() env-override idiom closely.
Medium	Quality	Changes are focused (single concern)	Pass	Two source files plus tests, all on one concern.
Low	Quality	Meaningful names, no dead code	Pass	bsScopeRootOverride / recursiveScope / scopedGlob read clearly.
Low	Quality	Comments explain why, not what	Pass	Comments are a strength — they carry rationale, not restatement.
Low	Quality	No unnecessary dependencies added	Pass	No new dependencies.

Findings

• File: packages/core/src/maestro-screenshot.js:132 (with packages/core/src/maestro-screenshot-file.js:81-150)
• Severity: High
• Reviewer: stack-code-reviewer
• Issue: When the override is active, scopeRoot is the raw env value and sessionId plays no part in narrowing the search — the glob is {root}/**/{name}.png and the walker's accept predicate is () => true. This matters because a single relay process serves multiple sessions concurrently: percy.maestroInsetCache is a per-process Map keyed by sessionId (packages/core/src/percy.js:155), which only makes sense if one process fields requests for several sessions. Since PERCY_MAESTRO_BS_SCOPE_ROOT is process-level, one value must span every session that process serves. Two consequences follow. (a) Two concurrent sessions using the same snapshot name — Home, Login, and similar are common — both match, and the newest-mtime tie-break at maestro-screenshot-file.js:142-150 picks between them with no session signal. The relay then returns a valid but wrong screenshot, which in a visual-diff product is worse than a 404: it produces a false diff or a false pass rather than an error. (b) On the filePath branch, containment now only proves the path is under the shared root, so a request carrying session A's id can successfully read session B's file. The PR body does address same-name collisions, but frames them as sequential reuse ("over the life of a device") where newest-mtime is a sound answer; the concurrent-sessions-in-one-process case is a different scenario and is not covered by that reasoning.
• Suggestion: Keep sessionId in the scope when both it and the override are present — e.g. scope to {overrideRoot}/{sessionId} when that directory exists, or have the host inject the session segment. If the intended contract really is "the host provisions this per-session", make that an enforced or at minimum explicitly documented precondition in the code and PR body, and add a regression spec with two session ids and the same name under one root that pins the intended behaviour.

---

• File: packages/core/src/maestro-screenshot-file.js:81 (effect at :118)
• Severity: Medium
• Reviewer: stack-code-reviewer
• Issue: The search-pattern branch now groups self-hosted and BS-override together via scopedGlob = selfHosted || !!recursiveScope, but the dot option is still keyed on selfHosted alone: glob(searchPattern, selfHosted ? { dot: true } : undefined). The comment justifying that ("BS layouts have no dot-prefixed segments") was true for the fixed _maestro_debug_ / test_suite/logs conventions but no longer holds once an operator can point the root anywhere. The result is a divergence between the two lookup paths: fast-glob silently skips dot-prefixed directories beneath the override root while manualScreenshotWalk — plain readdir — would find them, so the fallback can succeed where the primary path 404s.
• Suggestion: Key the option on the same predicate as the pattern: glob(searchPattern, scopedGlob ? { dot: true } : undefined), and update the adjacent comment to say why.

---

• File: packages/core/src/maestro-screenshot-file.js:171
• Severity: Low
• Reviewer: stack-code-reviewer
• Issue: The containment-failure message renders the root as selfHosted ? 'PERCY_MAESTRO_SCREENSHOT_DIR' : 'session dir'. With the override active the root is neither — it is the operator-supplied PERCY_MAESTRO_BS_SCOPE_ROOT — so a symlink-escape or a misconfigured root reports "resolved outside session dir", pointing on-call at the wrong variable.
• Suggestion: Thread a label through: selfHosted ? 'PERCY_MAESTRO_SCREENSHOT_DIR' : recursiveScope ? 'PERCY_MAESTRO_BS_SCOPE_ROOT' : 'session dir'.

---

• File: packages/core/src/maestro-screenshot-file.js:101
• Severity: Low
• Reviewer: stack-code-reviewer
• Issue: {globRoot}/**/{name}.png places no depth or breadth bound on the traversal, while the manual walker it falls back to caps at depth > 15. The gap predates this PR on the self-hosted path, but the PR extends the unbounded pattern to BrowserStack traffic, where request volume is far higher and the root is chosen by host config rather than by the customer.
• Suggestion: Pass fast-glob's deep option to match the walker's cap, so both paths agree on traversal bounds.

---

Noted, not raised as a defect

• No existence pre-check on the override root. The self-hosted branch 400s on a missing PERCY_MAESTRO_SCREENSHOT_DIR while the override falls through to a generic 404. The reviewer flagged the asymmetry, but the PR body argues it deliberately — customer-fixable config warrants a 400, host config does not — and that reasoning holds. Adding a debug log line naming the root on the 404 path would still help operators without changing the status code.

Verification notes

• Finding 1's premise was checked independently against the code rather than taken on the reviewer's word: this.maestroInsetCache = new Map() at packages/core/src/percy.js:155, keyed by sessionId at maestro-screenshot.js:183-186, confirms multi-session-per-process.
• Findings 2 and 3 were confirmed by reading the current lines (:118, :171).
• PERCY_MAESTRO_BS_SCOPE_ROOT is undocumented in markdown, but so are its siblings PERCY_APP_AUTOMATE_TMP_DIR and PERCY_MAESTRO_SCREENSHOT_DIR — consistent with existing convention, so not raised as a finding.

---

Verdict: FAIL — one High finding (session isolation collapses under the override). The override mechanism itself is sound; it needs sessionId to stay in scope, or the per-session precondition made explicit and enforced.

[pranavz28]

Claude Code Review (automated) — 2 inline finding(s). Full report in the PR comment below. Verdict: Passed.

[pranavz28]

[Low] deep bound may sit one level short of the walker's cap, and newly bounds the self-hosted path

Two small follow-ups on the new cap — the original "unbounded glob" finding is resolved by this line.

1. deep: MAX_SEARCH_DEPTH and the walker's if (depth > MAX_SEARCH_DEPTH) return are reported not to be equivalent: testing fast-glob 3.x, deep: 15 reaches one directory level less than the walker does. A file at exactly the boundary depth would then be found by the manual-walker fallback but missed by the primary glob. Flagging the provenance honestly — this is the reviewer's empirical result; I was not able to re-run the check myself.
2. deep now also applies to the self-hosted path, which previously had no cap. A customer whose --test-output-dir nests deeper than the cap would newly get a 404 instead of a hit.

Suggestion: use deep: MAX_SEARCH_DEPTH + 1, or add a comment recording that the two bounds intentionally differ by a level. The self-hosted cap is worth a release-note line — it is a behaviour change on an existing path, even though it aligns that path with the walker's long-standing limit.

Reviewer: stack-code-reviewer

[pranavz28]

[Low] New stat TOCTOU lacks the explanatory comment its sibling carries

This stat sits in the same shape as the self-hosted PERCY_MAESTRO_SCREENSHOT_DIR stat a few lines above, which carries an explicit comment on why the race is acceptable (realpath, not stat, is the security boundary).

Functionally this is fine: the stat only chooses which string becomes scopeRoot, and locateScreenshot still realpaths both sides before the prefix check. But a future reader has to re-derive that, and could mistake it for a security control.

Suggestion: one line mirroring the existing comment at maestro-screenshot.js:116-121.

Reviewer: stack-code-reviewer

[pranavz28]

Claude Code PR Review

PR: #2404 • Head: 2180cb5 • Reviewers: stack-code-reviewer

Continues the previous review — changes since 862606e (delta).

Summary

Three commits since the last review (bd5a3aa, 494da75, 2180cb5) address all four previously-open findings: sessionId is back in the scope where the injected root permits it, dot is keyed on scopedGlob, both root-recursive globs are bounded by a shared MAX_SEARCH_DEPTH, and the containment error names the variable actually in play. Semgrep's path-join-resolve-traversal alerts on the new code were closed by building the session-scoped root with string concatenation instead of path.join; both scanner threads are now resolved and Semgrep/CodeQL are green.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	Unchanged.
High	Security	Authentication/authorization checks present	N/A	No auth surface in this change.
High	Security	Input validation and sanitization	Pass	sessionId is SAFE_ID-validated (/^[a-zA-Z0-9_-]+$/, maestro-screenshot.js:48-54) before it is concatenated into the scoped root, so the concat cannot traverse.
High	Security	No IDOR — resource ownership validated	Pass	Was the gating failure last round. Now: when {root}/{sessionId} is a directory the scope narrows to that subtree, and a filePath into a sibling session is rejected (api.test.js "rejects a filePath from another session subtree once narrowed"). The residual un-narrowed case is a documented host contract — see Findings.
High	Security	No SQL injection (parameterized queries)	N/A	No SQL.
High	Correctness	Logic is correct, handles edge cases	Pass	Narrowing, non-directory fallback, dot-dirs, trailing slash, non-absolute, missing root all covered by specs.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	scopeLabel now names the right variable; the catch around stat deliberately degrades to the un-narrowed root.
High	Correctness	No race conditions or concurrency issues	Pass	The stat→realpath window only selects which string becomes scopeRoot; realpath remains the enforcement point. Same shape the self-hosted branch already documents.
Medium	Testing	New code has corresponding tests	Pass	Five specs added this delta (narrowing, filePath rejection, non-directory session entry, dot-dir, shared-name tie-break).
Medium	Testing	Error paths and edge cases tested	Pass	Fixing the error label exposed that the composed-BS session dir arm had no coverage; a spec was added for it.
Medium	Testing	Existing tests still pass (no regressions)	Pass	45 checks green. Both Test @percy/core legs were still running at review time; Semgrep OSS, semgrep/ci, CodeQL and Analyze all pass.
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	Both root-recursive globs now bounded by deep: MAX_SEARCH_DEPTH; see Finding 1 for the boundary detail.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable.
Medium	Quality	Follows existing codebase patterns	Pass	MAX_SEARCH_DEPTH shared between glob and walker; label branch mirrors existing style.
Medium	Quality	Changes are focused (single concern)	Pass	Three commits, each one concern.
Low	Quality	Meaningful names, no dead code	Pass	MAX_SEARCH_DEPTH, sessionScoped, scopeLabel all read clearly.
Low	Quality	Comments explain why, not what	Pass	The stale dot comment was removed rather than reworded, which is right. One gap — see Finding 2.
Low	Quality	No unnecessary dependencies added	Pass	None.

Resolved since 862606e

• packages/core/src/maestro-screenshot.js:132 High — Override collapses per-session isolation into a shared search root Resolved in 494da75 / 2180cb5. The finding asked for one of two things: keep sessionId in scope when the override is present (suggested literally as "{overrideRoot}/{sessionId} when that directory exists"), or make the per-session precondition explicit and documented. Both were delivered — the conditional narrowing is implemented and tested, and the PR body now states the precondition in full, including what a host that cannot meet it should inject instead. Residual risk is recorded below as Finding 3 rather than carried as a High.
• packages/core/src/maestro-screenshot-file.js:81 Medium — dot gated on selfHosted rather than scopedGlob Resolved in bd5a3aa — now scopedGlob ? { dot: true, deep: MAX_SEARCH_DEPTH } : undefined, covered by the dot-prefixed-directory spec.
• packages/core/src/maestro-screenshot-file.js:171 Low — error text says "session dir" under the override Resolved in bd5a3aa via scopeLabel.
• packages/core/src/maestro-screenshot-file.js:101 Low — recursive glob had no depth cap Resolved in bd5a3aa — deep: MAX_SEARCH_DEPTH, shared with the walker's cap. The boundary detail is carried forward as a new Low (Finding 1).

Findings

• File: packages/core/src/maestro-screenshot-file.js:116
• Severity: Low
• Reviewer: stack-code-reviewer
• Issue: Two small things around the new cap. First, deep: MAX_SEARCH_DEPTH and the walker's if (depth > MAX_SEARCH_DEPTH) return are reported not to be equivalent — the reviewer tested fast-glob 3.x and found deep: 15 reaches one directory level less than the walker does, so a file at exactly the boundary depth would be found by the fallback but missed by the primary glob. I was not able to run that check myself (the sandbox denied the command), so this is the reviewer's empirical result rather than something I confirmed. Second, deep now also applies to the self-hosted path, which previously had no cap at all — a customer whose --test-output-dir nests deeper than the cap would newly get a 404.
• Suggestion: Either use deep: MAX_SEARCH_DEPTH + 1 or add a comment recording that the two bounds intentionally differ by a level. Worth a line in the release notes for the self-hosted cap, since it is a behaviour change on an existing path even though it aligns that path with the walker's long-standing limit.

---

• File: packages/core/src/maestro-screenshot.js:133
• Severity: Low
• Reviewer: stack-code-reviewer
• Issue: The new stat(sessionScoped) sits in the same TOCTOU shape as the self-hosted PERCY_MAESTRO_SCREENSHOT_DIR stat a few lines above, which carries an explicit comment explaining why the race is acceptable (realpath, not stat, is the security boundary). The new one has no such comment. It is functionally fine — this stat only chooses which string becomes scopeRoot, and locateScreenshot still realpaths both sides — but a future reader has to re-derive that, and may mistake it for a security control.
• Suggestion: One line mirroring the existing comment at maestro-screenshot.js:116-121.

---

• File: packages/core/src/maestro-screenshot.js:136
• Severity: Low
• Reviewer: orchestrator (residual from the resolved High)
• Issue: The narrowing is conditional: when {root}/{sessionId} is not a directory the scope stays the whole injected root. That is the case for the per-device layout this feature exists for, so in the target deployment the search space is the full root and isolation rests on the documented precondition rather than on code. The CLI cannot verify that precondition — no check it could run would establish "this root contains only my session's data" — so documentation plus opportunistic narrowing is the practical ceiling here, which is why this is recorded as residual rather than as an open defect.
• Suggestion: Nothing required for this PR. If App Automate's allocation model ever changes so a root can span concurrent sessions, the narrowing should become mandatory (missing subdirectory → 404) rather than a fallback.

Reviewer disagreement, and how it was settled

stack-code-reviewer returned REQUEST CHANGES, holding that the High finding is still open because the narrowing does not fire for the sessionId-less target layout, and that the new spec resolves a name shared by two session subtrees to the newest file "codifies the leak as expected behaviour".

Both of those observations are factually correct — I verified them. SID is 'testsession' (api.test.js:1294), {SCOPE_ROOT}/testsession does not exist in that spec, so narrowing does not fire and the assertion does return the newer sibling subtree's file.

I did not carry the High forward, for two reasons:

1. The remediation the finding asked for was delivered. The original finding's suggestion named the conditional narrowing explicitly — "{overrideRoot}/{sessionId} when that directory exists" — or an explicitly documented precondition. The author implemented both and added specs. Escalating now to a stricter fix (hard 404 on a missing session subdirectory) that the finding never asked for would be moving the goalposts after the bar was met.
2. The disputed spec pins a tie-break, not a security claim. Under the stated contract — the injected root contains only what this session may read — sibling subtrees under that root are readable by definition, so the spec is documenting which of several in-contract matches wins, deterministically. The reviewer's suggestion to make it assert a 404 would be right only if the contract were the opposite one.

What the reviewer is really disagreeing with is the contract, not the code. That is a legitimate design question, and I have recorded it as Finding 3 so it is not lost — but it turns on an App Automate deployment property (CLI started per session, device allocated exclusively) that cannot be verified from this repo and that the code owner has asserted explicitly in the PR body. That is not a basis for a red gate.

---

Verdict: PASS — all four prior findings addressed; three Low items remain, none gating.