diff --git a/hypaware-core/plugins-workspace/codex/src/exchange-projector.js b/hypaware-core/plugins-workspace/codex/src/exchange-projector.js index cdab2da1..eef3fe6d 100644 --- a/hypaware-core/plugins-workspace/codex/src/exchange-projector.js +++ b/hypaware-core/plugins-workspace/codex/src/exchange-projector.js @@ -1,7 +1,6 @@ // @ts-check -import { isAbsolute } from 'node:path' - +import { sessionMetaCwd } from '../../../../src/core/codex/rollout_session_meta.js' import { createUsagePolicyResolver, isEqualOrDescendant, USAGE_POLICY_DROP } from '../../../../src/core/usage-policy/index.js' import { redactRemoteUserinfo } from './git-remote.js' import { @@ -1214,12 +1213,21 @@ function readRecordedCwd(reqBody) { * A drop that only holds while the daemon runs from the right directory is what * fail-closed would have to replace, and that is the larger call above. * - * The same two checks guard the rollout-stated cwd as `sessionMetaCwd` - * (`src/core/codex/rollout_session_meta.js`, LLP 0150 `#usable-cwd`). They are - * restated here rather than borrowed from there: LLP 0150 scopes the in-band path - * out of its own mandate (in-band is a separate source, and its value is also - * stamped on the row), and the `error_kind` split below needs the two conjuncts - * apart, which that predicate's single answer does not give. + * The answer itself is not computed here. It comes from `sessionMetaCwd` + * (`src/core/codex/rollout_session_meta.js`, LLP 0150 `#usable-cwd`), the one + * predicate that also guards the rollout-stated cwd, so the two sources that + * feed this gate cannot answer "is this a usable container?" differently. What + * stays local is only the refusal *diagnosis*: `error_kind` needs blank told + * apart from relative, and a single `undefined` does not carry that. Splitting + * the conjuncts for the log is safe because they are not the verdict; the + * verdict is whatever the shared predicate returned. + * + * LLP 0150 scoped the in-band path out of its own mandate (in-band is a separate + * source with its own trust story, and its value is also stamped on the row), so + * this reuse is a de-duplication rather than an invariant it imposed (#478). It + * changes no behavior: only a non-empty string reaches here (`readStringKey` and + * `firstString` refuse the rest upstream), and over that whole domain the two + * former copies agreed. * * One thing this does NOT reach, so nobody reads it as the whole gate: when the * request states no `cwd` at all, the value passed in is the workspace key @@ -1236,6 +1244,8 @@ function readRecordedCwd(reqBody) { * * @ref LLP 0083#decision [implements]: an unusable in-band cwd counts as a miss, * so the rollout fallback still gets its turn + * @ref LLP 0150#usable-cwd [implements]: both cwd sources for this gate share + * the one predicate; this wrapper adds only the refusal diagnosis * @param {string | undefined} cwd * @param {{ * log?: { @@ -1247,12 +1257,11 @@ function readRecordedCwd(reqBody) { */ function usableInBandCwd(cwd, ctx) { if (cwd === undefined) return undefined - // Byte-identical when it passes: the trim is only the emptiness test, and a - // path is not ours to normalize. The trim gates nothing on its own - a blank - // string is never absolute on either platform, so `isAbsolute` already refuses - // it - it is here to split `error_kind` below, which is the only thing that - // tells blank apart from relative. Keep both conjuncts or that split dies. - if (cwd.trim().length > 0 && isAbsolute(cwd)) return cwd + // Byte-identical when it passes: `sessionMetaCwd` trims only to test + // emptiness and returns the value unchanged, because a path is not ours to + // normalize. + const usable = sessionMetaCwd(cwd) + if (usable !== undefined) return usable // Never silently: a refused cwd means this exchange reached the gate with // nothing to match, which is indistinguishable from "no cwd at all" in the // row. Hash it - a cwd is user data (LLP 0049), and the drop log above uses @@ -1262,6 +1271,9 @@ function usableInBandCwd(cwd, ctx) { // refused upstream with no log. Same outcome (absent cwd, NULL column, and it // was already absent before this predicate existed), no diagnostic. Closing it // means loosening a helper with 16 other callers here, which is not worth it. + // The trim below re-tests a conjunct the shared predicate already weighed, but + // it decides nothing: the refusal is settled above. It only says WHICH way the + // value was unusable, which is the one thing a single `undefined` cannot carry. ctx?.log?.warn?.('plugin.codex.usage_policy_cwd_unusable', { component: 'codex', operation: 'usage_policy_cwd', diff --git a/llp/0083-codex-live-cwd-from-rollout.decision.md b/llp/0083-codex-live-cwd-from-rollout.decision.md index 6102d0c8..ffc2a18a 100644 --- a/llp/0083-codex-live-cwd-from-rollout.decision.md +++ b/llp/0083-codex-live-cwd-from-rollout.decision.md @@ -70,21 +70,29 @@ has the symmetric fallback. against the **daemon's** own process cwd and return a confident verdict for an unrelated directory (#471). The rollout fallback then still gets its turn, and when nothing usable is found the row records `cwd = NULL` exactly as before, so - refusing does not make this path fail closed. The rollout-stated `cwd` is held - to the same two checks, by a different owner: core's `sessionMetaCwd` refuses a - blank or relative `session_meta.cwd` + refusing does not make this path fail closed. Both cwd sources for this gate + answer that from **one** predicate: core's `sessionMetaCwd` refuses a blank or + relative value ([LLP 0150 §usable-cwd](./0150-one-reader-for-codex-session-meta.decision.md#usable-cwd)), - and `rollout-cwd.js` reads through it, so a refused in-band value falls through - to an already-predicated source. That predicate is **not** borrowed for the - in-band value, and this bullet is not a consequence of LLP 0150: 0150 scopes the - in-band path out of its own mandate, because in-band is a separate source with - its own trust story whose value is also stamped on the row for workspace/git - enrichment. So the two checks are restated locally, in `usableInBandCwd`. One - limit of the rule, stated rather than implied: on the Codex route the value the - predicate sees is usually not the request's `cwd` but the workspace key - `selectCodexWorkspace` selected for it, which substitutes the first workspace - when none matches, so an absolute-but-unrelated directory can still reach the - gate (#476). + `rollout-cwd.js` reads the rollout through it, and `usableInBandCwd` calls it + for the in-band value, so a refused in-band value falls through to a source + predicated the same way. The in-band wrapper survives only to name *which* way + the value was unusable (`error_kind`: `cwd_blank` vs `cwd_not_absolute`), which + a single `undefined` cannot carry. Sharing the predicate is a de-duplication + (#478), not an invariant LLP 0150 imposed: 0150 scopes the in-band path out of + its own mandate, because in-band is a separate source with its own trust story + whose value is also stamped on the row for workspace/git enrichment. The first + cut (#471) restated the two checks locally on exactly that scoping argument, + not for want of an owner: `sessionMetaCwd` was already on `master` when that + copy landed, and its docstring cited LLP 0150 `#usable-cwd` by anchor while + declining to borrow it. What changed here is the weight given to drift, not + the scoping: the two readings were behaviourally identical over every input + that can reach the seam, so folding them together changed nothing except the + number of places the rule can drift from. One limit of the rule, stated rather + than implied: on the Codex route the value the predicate sees is usually not + the request's `cwd` but the workspace key `selectCodexWorkspace` selected for + it, which substitutes the first workspace when none matches, so an + absolute-but-unrelated directory can still reach the gate (#476). - **Keyed on the codex thread id, and the rollout must confirm it.** A rollout is one **thread's** file: its name embeds `session_meta.payload.id` (the thread), matched via the `sessionIdFromPath` helper shared with the backfill (a helper diff --git a/llp/0150-one-reader-for-codex-session-meta.decision.md b/llp/0150-one-reader-for-codex-session-meta.decision.md index 093e366a..7edeb8f1 100644 --- a/llp/0150-one-reader-for-codex-session-meta.decision.md +++ b/llp/0150-one-reader-for-codex-session-meta.decision.md @@ -211,12 +211,19 @@ resolving on half a record. diverge is the *test* for whether the field is there, which is why the raw-line read and the blank rule are the same on both sides even though the fallback differs. -- **Still outstanding:** the live projector's *in-band* cwd - (`x-codex-turn-metadata` / body `metadata.cwd`, LLP 0083's fast path) reaches - the same `resolver.resolve` with no such predicate. It is a different source - with a different trust story, and its value is also stamped on the row for - workspace/git enrichment rather than only consulted for the gate, so tightening - it is its own decision rather than a consequence of this one. +- **The in-band cwd now shares the predicate too, by its own decision.** The + live projector's *in-band* cwd (`x-codex-turn-metadata` / body `metadata.cwd`, + LLP 0083's fast path) reaches the same `resolver.resolve`. This document + scoped that path out of its own mandate, and still does: it is a different + source with a different trust story, and its value is also stamped on the row + for workspace/git enrichment rather than only consulted for the gate, so + tightening it was its own decision + ([LLP 0083](./0083-codex-live-cwd-from-rollout.decision.md), #471) rather than + a consequence of this one. Having made it, `usableInBandCwd` calls + `sessionMetaCwd` rather than restating the rule (#478), keeping only the + `error_kind` split a single `undefined` cannot carry. So all three sites that + feed the gate now read the rule from here, which is the point: agreeing by + construction is what two copies could not be relied on to do. - Code that lands this carries `@ref LLP 0150` on the reader and on both callers' seams. - Nothing about which id either caller *uses* changes here. This is the diff --git a/test/plugins/codex-exchange-projector.test.js b/test/plugins/codex-exchange-projector.test.js index b9b8d588..1d74cdb8 100644 --- a/test/plugins/codex-exchange-projector.test.js +++ b/test/plugins/codex-exchange-projector.test.js @@ -10,6 +10,7 @@ import { createCodexExchangeProjector, } from '../../hypaware-core/plugins-workspace/codex/src/exchange-projector.js' import { createAiGatewayMessageProjector } from '../../hypaware-core/plugins-workspace/ai-gateway/src/message_projector.js' +import { sessionMetaCwd } from '../../src/core/codex/rollout_session_meta.js' import { createUsagePolicyResolver, USAGE_POLICY_DROP } from '../../src/core/usage-policy/index.js' /** @@ -46,6 +47,17 @@ function clampingResolver(ignoredDir) { }) } +/** + * A real usage-policy resolver over an fs that holds no `.hypignore` anywhere. + * For a test about the cwd *predicate* rather than the gate: without it the + * matcher walks the real ancestors of the fixture path, so a `.hypignore` + * sitting above the checkout on whatever machine runs the suite would turn the + * assertion into a drop. + */ +function governsNothingResolver() { + return createUsagePolicyResolver({ existsSync: () => false }) +} + // @ref LLP 0050 [tests]: capture-seam drop: an ignored cwd yields no rows so // the gateway write guard persists nothing; a clean cwd is unaffected (R1/R2). test('project() returns no projection when the exchange cwd is .hypignore-ignored', () => { @@ -225,6 +237,41 @@ test('project() logs an unusable in-band cwd rather than skipping the gate silen ) }) +// @ref LLP 0150#usable-cwd [tests]: the in-band seam and the rollout-stated +// seam are asked the same question and must answer it identically. Two copies +// of this exact rule have already drifted and shipped the wrong answer twice +// (#453, #459), so pin agreement at the seam rather than trusting two +// independent readings of the same two conjuncts (#478). +test('the in-band cwd seam answers exactly as the shared sessionMetaCwd predicate does', () => { + // The whole of what the in-band predicate can be asked: only a non-empty + // string reaches it (`readStringKey` and `firstString` both refuse anything + // else upstream), so blank-ish, relative and absolute forms are the cases. + const cases = [ + '/work/repo', // absolute: accepted, byte-identical + '/work/repo/', // trailing slash is not ours to normalize + '/work/repo/./sub', // an unnormalized absolute path is still absolute + 'repo', // bare relative + './repo', + '../elsewhere', + ' ', // blank after trim + '\t\n', + ' /work/repo', // non-blank but not absolute: the trim alone would accept it + ] + const projector = createCodexExchangeProjector({ resolver: governsNothingResolver() }) + for (const cwd of cases) { + const projection = /** @type {any} */ (projector.project(exchange({ + path: '/v1/chat/completions', + request_body: JSON.stringify({ cwd, messages: [{ role: 'user', content: 'hi' }] }), + response_body: JSON.stringify({ choices: [{ message: { role: 'assistant', content: 'ok' } }] }), + }), context())) + assert.equal( + projection.cwd, + sessionMetaCwd(cwd), + `${JSON.stringify(cwd)}: the in-band seam must not be looser or stricter than sessionMetaCwd`, + ) + } +}) + // --------------------------------------------------------------------- // Session opt-out (LLP 0066): a second, independent match key at the same // USAGE_POLICY_DROP seam, keyed on the STAMPED session_id