Codex live projector: an explicit cwd outranks a substituted workspace key at the .hypignore gate - #477
Conversation
…e key at the .hypignore gate (#476) `selectCodexWorkspace` falls back to the first `workspaces` turn-metadata key when none matches the request's `cwd`, and `resolveCodexContext` stamped that result as the one resolved `cwd` (LLP 0083) that feeds the `.hypignore` gate. So when the request's real cwd matched no declared workspace, the privacy verdict was computed for an unrelated directory: an opted-out session could be recorded (the leak), and a session no `.hypignore` covered could be dropped. An explicit in-band `cwd` now outranks the workspace key for the gate and the stamp. The key keeps its enrichment role and still supplies the `cwd` on the subscription route, where the request states none and the key is the only in-band source there is. A refused substitution is reported as `plugin.codex.usage_policy_workspace_cwd_refused` with hashed paths. `selectCodexWorkspace` itself is untouched to keep the conflict surface with PRs #462, #467 and #474 as small as possible. Co-Authored-By: Claude <noreply@anthropic.com>
…view) Review of PR #477 verified by execution that the fix closes #476 case (a) and several unreported variants of it, and found two consequences the amendment did not state: - the workspace key still outranks the rollout fallback, so a subscription-route session that declares a `workspaces` map never consults `session_meta.cwd` and a first-key guess can still decide its verdict (true on `master` too); - because the key keeps enriching, a row recorded where it used to drop (clean in-band cwd, ignored declared workspace) carries that ignored workspace's identity. Doc only: no code change, so the projector's contended lines are untouched. Co-Authored-By: Claude <noreply@anthropic.com>
…view) Review of PR #477 verified by execution that the fix closes #476 case (a) and several unreported variants of it, and found two consequences the amendment did not state: - the workspace key still outranks the rollout fallback, so a subscription-route session that declares a `workspaces` map never consults `session_meta.cwd` and a first-key guess can still decide its verdict (true on `master` too); - because the key keeps enriching, a row recorded where it used to drop (clean in-band cwd, ignored declared workspace) carries that ignored workspace's identity. Doc only: no code change, so the projector's contended lines are untouched. Co-Authored-By: Claude <noreply@anthropic.com>
4c23c17 to
36ebf64
Compare
Review of PR #477 verified by execution that the fix closes #476 case (a) and several unreported variants of it, and found three consequences the amendment did not state: - the workspace key still outranks the rollout fallback, so a subscription-route session that declares a `workspaces` map never consults `session_meta.cwd` and a first-key guess can still decide its verdict (true on `master` too); - because the key keeps enriching, a row recorded where it used to drop (clean in-band cwd, ignored declared workspace) carries that ignored workspace's identity; - the gate does not canonicalize, so a symlinked in-band spelling of an ignored directory is now recorded where the key's canonical spelling used to drop it. Doc only: no code change, so the projector's contended lines are untouched. Co-Authored-By: Claude <noreply@anthropic.com>
36ebf64 to
0db67e1
Compare
Neutral review, round 1 of 2Reviewed head Verdict: findings. The central claim holds: case (a) is closed, and closed 1. Case (a) is closed, and so are four variants the body does not mention
Four rows besides (a) flip from a leak to a correct drop, because On the header- Row-stamp change (undisclosed change 2) is contained. 2. FindingsF1. MEDIUM (privacy, not closed by this PR, also true on
Row 1 versus row 2 is the finding: adding a F2. MEDIUM (privacy, new in this PR, a net loss versus
The symlink jumps depth, so the ancestor walk from the symlinked spelling never F3. LOW-MEDIUM (privacy, new). The gate is scoped by F4. LOW (observability). F5. INFO. A whitespace-only in-band F6. INFO. Once both land, PR #474's LLP 0083 paragraph still says the #476 F7. INFO, not this PR's. 3. Claims I re-derived
4. What I changedOne commit, doc only, pushed to 5. Decisions for a human
None of D1-D4 blocks landing. F1 and F2 are the two I would not leave |
…ssues (#476 review round 2) Round 2 of the #477 review filed each residual as its own issue so it does not live only in this paragraph: #480 (the key preempts the rollout session_meta.cwd, pre-existing), #481 (a newly-recorded row carries an ignored workspace's identity), #479 (the shared matcher never canonicalizes, so a symlinked spelling of an ignored directory escapes its .hypignore). Also corrects the third limit. It was written as a trade this amendment makes; execution against a real on-disk symlink shows it is a property of the shared matcher that predates the amendment, that the amendment swaps which of two symmetric spellings trips it rather than opening a new leak, and that the widest case (a declared symlinked key with no in-band cwd at all) is identical before and after. Notes that the matcher fix must canonicalize the local-only list entries too, since canonicalizing only the incoming cwd un-governs an entry a user marked by its symlink spelling. Doc only: exchange-projector.js stays byte-identical, so the conflict surface against #462 and #474 is unchanged. Co-Authored-By: Claude <noreply@anthropic.com>
Neutral review, round 2 of 2 (final)Reviewed head Verdict: findings. Nothing found is a reason to hold the PR, and the central 1. F2, the symlink leak: reachable, but NOT a net regressionReachable: yes. Same directory, opposite verdicts.
Round 1 measured A only, and concluded "a net loss versus Which of A/B is likelier is the real question, and it favours the PR. POSIX Not fixable in this PR, and now demonstrably so. I built the fix and measured
Filed as #479, 2. F1: confirmed pre-existing, byte-identical, now tracked
Identical on 3. D3 and D4: verified, left unfixed, filed as #481Both confirmed by execution, both branch-only, so #481 carries D3, LOW-MEDIUM, privacy (identity, not content). D4, LOW, observability. 4. Semantic drift against the moved
|
|
Triage of PR #477 at head Round 2's commit ( Verdict on the round-1-vs-round-2 reversal: round 2 is correct. I did not take its word for it — I built real on-disk symlinks (
This is exactly the symmetric swap the doc claims, not a net loss: the PR closes the case where the declared workspace key carries a non-canonical spelling and opens the case where the request's own cwd does. The reason this favors the branch is structural, not hopeful: I also spot-checked the four round-1 "master leaks" variants by direct execution (real projector, stub resolver): trailing-slash, dot-segment, and LLP 0083 text at Residuals — confirmed accurately described and correctly labelled, not re-filed:
None of the three is mislabelled. D3 / enrichment-identity leak (#481 D1) — judged non-blocking, tracking in #481 is sufficient. It leaks only identity (a workspace path string, a redacted-userinfo git remote, a commit sha), never session content, and only in a narrow multi-workspace shape (a clean real cwd alongside a different, ignore-flagged declared workspace key). On master this exact row never existed at all — but only because master over-drops it (case (b): master would have wrongly suppressed a legitimate, non-ignored session). The counterfactual that matters here: holding this PR back to close a low-severity identity leak leaves case (a) — a full content record of a session that should have been dropped — shipping in production. That trade isn't close. #474 composition re-verified at its current head ( Ref hygiene:
Every residual finding here is non-blocking. Follow-ups already exist (#479/#480/#481); no new issue filed. |
Three files conflicted. Each resolution is a union of both intents, not a side-picking, because the two sides changed different things about the same subject (Codex session identity and the opt-out caveat). session_command.js: keep this branch's EPHEMERAL_NOTE constant (#455: the caveat names the fork as well as the restart) and master's object-argument provenanceNotes({ idSource, idEvidence, threadId, endpointSource }) call. Only one of the two call sites conflicted; the other already carried the combination, so this makes the two branches of the if/else identical again. types.d.ts: keep master's prose, which is the correct one now - it explains sessionId as the session container versus threadId, and the merged union already carries master's rename of codex_env to codex_env_rollout, which this branch's prose still called codex_env. Re-attached this branch's gloss on the @ref LLP 0067#cli-session-id annotation, which master had left bare. Anchor {#cli-session-id} verified present at llp/0067:305. codex privacy skill SKILL.md: this branch's rollout-selection policy wins (match payload.cwd, refuse on zero/ambiguous/stale, never newest-by-mtime - issue #452), since master's side still said "pick the newest rollout". Folded master's readRolloutMeta-mirroring guards into that same walk rather than dropping them: the session_meta type check and payload-dict check (as skips, since this side walks many files), plus the non-string and whitespace-in-id refusals, which this side needs too because it reads the result through `read -r`. Took master's `hyp session ignore --json` over the bare verb. Semantics checked, not just textual cleanliness: - The claim "the gateway's drop keys on the container" is STILL TRUE after PR #477. exchange-projector.js line 113 is unchanged: `const sessionId = stringValue(codexContext?.session_id) ?? conversationId`, i.e. metadata.session_id falling back to the conversation (thread) id, exactly as the skill prose and the test comment describe. #477 (bcad4a4) only changed the separate .hypignore gate, which keys on a cwd path, not on any session id. So no correction was needed there. - The three-way agreement still holds between the CODEX_THREAD_ENV comment in session_command.js (master renamed it from STATED_SESSION_ID_VARS), the skill's Step 1 prose, and the drop code: thread id is a selector, the container is the answer. - Corrected one thing that HAD gone stale: this branch's prose said issue #453 "puts CODEX_THREAD_ID to its real use" in the future tense, but #453 shipped on master (5d270a5, c551d6e). The prose now describes the codex_env_rollout source as live, matching llp/0067:317 ("selector, not an answer") and llp/0067:585. - The fork caveat agrees across all three surfaces it appears on: EPHEMERAL_NOTE, the Codex skill, and the Claude skill (`claude --fork-session` / `codex fork`). The usage-policy unification (#482/#484, fold(realpath(p))) touches no file on this branch and needed no reconciliation here. Checks: npm test 3041 tests, 8 failures, all of them the pre-existing test/core/leave-command.test.js "leave ..." set, name-for-name identical to a pristine origin/master run (3037 tests, same 8). Test count rose by 4, which is this branch's new tests passing. npm run typecheck clean. Co-Authored-By: Claude <noreply@anthropic.com>
A semantic merge, not a textual one: master reworked the same Codex cwd path under this branch's feet (#466 LLP 0150, #467 LLP 0151, #474, #477), so the conflicts had to be resolved on what the combined behaviour means, not on which side's hunk looked cleaner. Four files, and what each side wanted: - exchange-projector.js: master wrapped the in-band cwd in `usableInBandCwd` (#474) and added the refused-workspace warn (#477), both around the very expression this branch replaced. Kept both, with this branch's `resolveRolloutCwd` as the fallback rather than master's inline `rolloutCwd.resolve(session_id)`, which is the container key #459 is about. - rollout-cwd.js: master replaced the local first-line read with core's one `readRolloutSessionMeta` (LLP 0150); this branch added a thread-identity guard on top of that read. Composed rather than chosen: the guard now compares `meta.threadId` from the shared reader. The two fit exactly, because LLP 0150 rule 1 (raw JSONL line, never Codex's `Deserialize`) is the property the guard depends on to see an absent `payload.id` as absent. `meta.cwd` also arrives pre-predicated by `sessionMetaCwd`, so a blank or relative rollout cwd is now refused here too. Cache key stays the thread id. - LLP 0083: took master's Context correction and its unusable-in-band bullet, kept this branch's thread-keying thesis over master's superseded "keyed on the codex session id" bullet, and reconciled the prose that #467 falsified: the thread now comes from the body's `client_metadata`, not from `thread-id` / `session-id` header names Codex never emitted. The Consequences bullet promising a shared-reader follow-up was stale (that fold has landed) and now says so. - test/plugins/codex-rollout-cwd.test.js: git merged this file cleanly and the result was wrong in both directions, which is the part worth reading. Master's #257 fixtures key the fake resolver on the session id while stating a distinct thread id, so thread keying missed; rekeyed onto the thread id, which keeps master's deliberately-distinct pair. More seriously, this branch's #459 fixtures state identity through the bare `session-id` / `thread-id` / `parent-thread-id` headers, which #467 established are names no Codex version emits and removed the reads for. Left alone, four leak-direction tests failed outright and the refusal tests would have passed VACUOUSLY, for want of any id rather than because a refusal fired, silently gutting the gate. Ported the fixtures to the body `client_metadata` surface (LLP 0151), assertions unchanged. Checked, not assumed: - Regression gate still bites: master's two source files under this merged test file fail 11 of 23, including every #459 leak-direction case and all four refusal cases, so the ported fixtures are not vacuous. - `npm test`: 3039 pass / 8 fail, exactly the `leave-command` 8 that fail identically on a pristine `origin/master` worktree (73b4618), by name. - `npm run typecheck`: clean. No em dashes, no semicolons in changed lines. The LLP anchors cited (0150#usable-cwd, 0151#body-is-authority, 0083#container-fallback-gap) all resolve. Not touched, deliberately: the open `subagent_signal` finding at `resolveRolloutCwd`. The refusal is still value-blind and its shape is unchanged, but #467 narrowed its reachability, since a turn now has to carry neither a Codex-owned `client_metadata` map nor a turn-metadata blob to reach the container fallback at all. LLP 0083 records that narrowing without pretending it closes the question. Co-Authored-By: Claude <noreply@anthropic.com>
… finding 2 of 2) (#491) * Codex workspace-cwd refusal is an ancestor test, not a byte test (#481) The refusal predicate was "a `workspaces` key was substituted and it is not byte-equal to the in-band cwd", so the completely ordinary shape "a session running in a subdirectory of its declared workspace" emitted `plugin.codex.usage_policy_workspace_cwd_refused` at `warn` on every single turn, with both directories clean and no `.hypignore` anywhere. A privacy warn that fires constantly on the common case is read as noise, and the signals beside it are read as noise with it. Narrow it to keys off the in-band cwd's ancestor chain. The justification is not "close enough": when the key is an ancestor, the cwd's `.hypignore` walk passes through the key and every machine-local entry governing the key also governs the cwd, so resolving the cwd is at least as restrictive as resolving the key would have been. The refusal can only tighten, so there is nothing to report. Off the chain the two walks are incomparable, in both directions - a sibling tree, and a key BELOW the cwd whose own walk covers strictly more - and those still warn. The test is the shared `isEqualOrDescendant` (LLP 0069 R8), not a second copy of the path rule. Lexical rather than spelling-agnostic on purpose: `scopeGoverns` buys its extra reach with realpath syscalls this per-exchange seam must not spend (LLP 0049 R6), and the residue errs toward reporting. LLP 0160 records the decision, the ancestor-monotonicity argument, and what is now stale in LLP 0083's #476 limit sentence; LLP 0083 gains the forward-ref. Does NOT address the other finding deferred from PR #477: a row recorded where it used to drop still carries an ignored workspace's identity through the key's surviving enrichment role. That is a privacy-relevant default the corpus does not settle, and it stays open under #481. Co-Authored-By: Claude <noreply@anthropic.com> * Review: the ancestor test's justification was a false monotonicity proof The behaviour is kept; the reason given for it was wrong and is replaced. LLP 0160, the `workspaceCoversCwd` JSDoc and the warn-site comment all claimed that when the key is an ancestor of the in-band `cwd`, `resolve(cwd)` is at least as restrictive as `resolve(key)`, so refusing the substitution "can only tighten". That is false on this resolver. Nearest-governs means a declaration between the key and the `cwd` overrides the key's and may be less restrictive. Swept against the real `createUsagePolicyResolver` over `.hypignore` bodies and machine-local list classes at four depths on one ancestor chain: 131 of 576 arrangements resolve the `cwd` LESS restrictively than the key, spanning ignore->local-only, local-only->full and ignore->full. Restated on the ground that actually holds: an ancestor key is not a guess about where the session ran, it is a less specific name for the same tree, and nearest-governs makes the `cwd`'s own declaration authoritative. So the signal reports the location inference, not a verdict change - and the residue (an ancestor key resolving more restrictively than its `cwd`, now silent) is disclosed rather than implicitly denied, and pinned by a test. Also corrected: - the symlink residue cited #479 as an open gap; #479 was closed for the gate by #482/#484. It is knowingly retained here, at this reporting-only predicate, and the converse direction (a lexical descendant that is really a symlink out of the tree reads as covered, so stays silent) was undisclosed. - LLP 0160 and LLP 0083 pointed the undecided enrichment question at #481, which this PR closes. It is split out to #492. Co-Authored-By: Claude <noreply@anthropic.com> * Review round 2: purge the disproved monotonicity claim from the places round 1 missed, and ground the retained justification by execution Round 1 replaced the false "an ancestor key can only tighten" proof in LLP 0160 §decision and in the `workspaceCoversCwd` JSDoc, but the same claim survived in three other places, one of them LLP 0160's own abstract, where it contradicted the §decision body directly: - `llp/0160-...md:13` - the summary blockquote still said "An ancestor key cannot have changed the `.hypignore` verdict". - `llp/0083-...md:144` - the `Extended-by` block said the same. - `test/plugins/codex-exchange-projector.test.js` - the `@ref LLP 0160#decision [tests]` block said "taking its own `cwd` over an ancestor key can only tighten the verdict". A ref that states a disproved premise is exactly what CLAUDE.md's "keep refs honest" rule is for. All three now state the ground that actually holds, and say explicitly that it is NOT a monotonicity argument. Round 1 also carried the disposition on a reasoned claim: "in every one of the 131 cases the loosening is the user's own nested declaration". Two changes: 1. A better, EXECUTED ground is now stated and pinned by a test. The warn this PR narrows carries no usage class at all. On `origin/master` it fires with an identical field set on a subdirectory turn with no `.hypignore` anywhere, on one where key and `cwd` both resolve `ignore`, and on the loosening arrangement. It could never have distinguished them, so narrowing it removes a constant, not privacy information. The genuine-refusal test now asserts the warn carries no `class`, `declared` or `governed_by`. 2. The provenance claim is corrected where it was too strong. The machine-local list, the only source that reaches an explicit `full`, has exactly two writers, both behind explicit `hyp ignore`/`unignore`/`policy set` verbs, and LLP 0071 §not-central forbids anything central writing one - so the `->full` transitions really are the user's own. But a `.hypignore` is a COMMITTABLE file by design (LLP 0071 §not-dotfiles) and the ancestor walk has no vendored-tree exclusion, so the `ignore->local-only` transition can come from a dependency's own file. Bounded (the walk only goes up, and `.hypignore` cannot express `full`), unchanged by this PR, and now disclosed rather than asserted away. Also notes that the 131/576 count's mixed slice is enumeration-dependent; the two source-pure slices, 50 and 75, reproduce exactly. No behaviour change: predicate, gate, row and drop path untouched. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: test <test@test.com> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: test <test@example.com>
Root cause
hypaware-core/plugins-workspace/codex/src/exchange-projector.jsselectCodexWorkspacepicks aworkspacesturn-metadata key, falling back tothe first key when none matches the request's
cwd:resolveCodexContextstamped that result ascodexContext.cwd, which is theone resolved
cwd(LLP 0083"One resolved
cwd, used twice") that feeds both the.hypignoregate and therow. So when the request's real
cwdmatched no declared workspace, the privacyverdict was computed for a substituted, unrelated directory, and because
workspacePaths[0]is normally absolute, PR #474'susableInBandCwdpredicateaccepts it and never sees the problem. The substitution also runs one layer
ahead of that predicate, so an unusable in-band
cwdnever reaches it.The design question, answered
A substituted workspace key is a guess about which directory the session ran
in, and a guess must not decide a privacy verdict. But it is not worthless: on
the ChatGPT-subscription route the request often carries no
cwdat all, and thekey is then the only in-band source of one. Deleting the substitution would
remove real
.hypignorecoverage.So the two roles are separated, which is the distinction the defect conflates:
"may we record this row at all" versus "what do we record about it".
Option chosen: option 1 (separate the roles)
An explicit in-band
cwdoutranks the workspace key for the gate and thestamp; the key keeps its enrichment role in full and still supplies the
cwdwhen the request states none.Chosen over the alternatives:
.hypignorecoverage, which is a net privacy loss. Rejected.
the same answer in all three reported cases but is worse in the common
benign one: it keeps gating on the workspace root and so keeps missing a
deeper
.hypignoreunder the actual cwd. Using the real cwd gates on the morespecific directory. It also needs a new ancestor helper for no benefit.
attributes.codex.workspace,git_remote,git_commitandhas_changesareuntouched and still come from the selected key, so no enrichment or graph-bridge
data (LLP 0032#capture) is lost.
selectCodexWorkspaceitself is not modified.Behaviour in the three reported cases
.hypignoreof classignoreat/work/ignored.workspacescwd{'/work/clean/proj':{}}/work/ignored/realcwd=/work/clean/proj{'/work/ignored/proj':{}}/work/clean/realcwd=/work/clean/real{'/work/ignored/proj':{}}subcwd=sub, refusal logged(a) is the privacy leak and it is closed. (b) is the false drop and it is closed.
Fail-open / fail-closed, explicitly
This fix fails OPEN, in case (c), and that is a trade, not a strict
improvement. Case (c) changes DROP to RECORD. The old drop was not a verdict,
it was luck: the guessed directory happened to be ignored. Refusing the guess
means the honest but unusable
subreaches the gate, where (on this base)path.resolvemeasures it against the daemon's own cwd. So this fix trades anaccidental drop for a fail-open record in the one case where the client states
a cwd we cannot use.
That is precisely the hole PR #474 closes, and the two compose: with #474
also landed, case (c) becomes "substitution refused,
subrefused as unusable,rollout fallback consulted, row recorded with
cwd = NULLplus ausage_policy_cwd_unusablewarn". This PR does not duplicate #474'spredicate, to keep the diff off the same lines.
Cases (a) and (b) are strictly better. One further consequence, deliberate and
recorded in the LLP:
metadata.cwdfrom the turn-metadata header now reaches thegate where only the workspace key did before, so a header-declared cwd under an
ignored tree will now correctly drop. And for a session running in a
subdirectory of its workspace the row stamps the subdirectory rather than the
workspace root, which is the directory the policy is actually scoped to.
I make no claim that this dominates every alternative.
Not silent
A refused substitution emits, at warn level, symmetric with #474's sibling
signal:
Paths are hashed, never raw: this seam sees LLM traffic. A test asserts the raw
path does not appear in the fields.
Reproducing tests
All in
test/plugins/codex-exchange-projector.test.js:the .hypignore gate uses the request cwd, not a substituted workspace key (#476 case a)an unrelated ignored workspace key does not drop a session it never covered (#476 case b)a refused workspace substitution is logged with hashed paths, not silently applied (#476 case c)a refused workspace substitution still enriches the row from the workspace key (#476)the workspace key still supplies the gate cwd when the request states none (#476)no workspace-cwd refusal is logged when the key matches or the request states no cwd (#476)1-4 fail on
origin/master(# tests 42 / # pass 38 / # fail 4; case (a)fails with a full projection where
USAGE_POLICY_DROPwas expected) and passafter. 5 and 6 encode behaviour that must survive the fix and are the
negative-branch guards.
Mutation checks
Every branch of the new logic was reverted individually:
cwd: workspace?.path(restore the substitution)cwd: inBandCwd(drop the workspace fallback)Codex turn metadata + headers project into first-class columns!pathsEqualguardinBandCwd &&guardVerification
npm test:# tests 2885 / # pass 2876 / # fail 8. The 8 failures are alltest/core/leave-command.test.jsand were verified to fail identically ona pristine
origin/masterworktree with the samenode_modulessymlink(baseline:
# tests 2879 / # pass 2870 / # fail 8, same 8 names).npm run typecheck: clean.npm run smoke -- gateway_codex_capture: ok.npm run smoke -- session_optout_capture_drop: ok.Diff size and conflict surface
3 files, +223 / -6. Of that, 38 lines in
exchange-projector.jsacross 4hunks, and 16 of those 38 are comment/
@reflines. The functional change istwo expressions plus one log block. 177 lines are tests, 14 are the LLP 0083
amendment.
exchange-projector.jsis concurrently modified by PRs #462, #467 and #474,so the conflict surface was deliberately minimised:
selectCodexWorkspaceisnot touched, nothing was refactored, reordered or renamed, #474's
usableInBandCwdis not duplicated or moved, and no line either of the heldPRs is likely to rewrite was reformatted. Per the issue, this should land
after #467 and #462 are resolved.
Fixes #476