Codex cwd resolution: a rollout with no thread id must count toward ambiguity, not vanish before the count (#499)

hypaware-core/plugins-workspace/ai-gateway/src/session_command.js

@@ -576,7 +576,13 @@ export function resolveSessionIdForCli(args) {
     }
   }
 
-  /** @type {{ threadId: string, sessionId: string | undefined, cwd: string | undefined, file: string }[]} */
+  // Recording the cwd is the whole of what makes a rollout a candidate here.
+  // Nothing further about it may filter the list, because the list is counted
+  // below and the count is the uniqueness claim: a header dropped for a field
+  // this path does not resolve on would take the ambiguity with it and leave a
+  // confident answer behind. Whether a candidate is usable is decided after the
+  // count, one candidate at a time.
+  /** @type {{ threadId: string | undefined, sessionId: string | undefined, cwd: string | undefined, file: string }[]} */
   const candidates = []
   for (const file of scan.files) {
     const meta = readRolloutMeta(file)
@@ -603,6 +609,27 @@ export function resolveSessionIdForCli(args) {
         error: `could not resolve a session id: the only Codex rollout recording cwd ${args.cwd} (${name}) was last written ${describeAge(ageMs)} ago, so it is a finished session rather than this one. Acting on it would report the WRONG session as covered while this one keeps being recorded. Pass the intended session id explicitly: hyp session status <session-id>.`,
       }
     }
+    // Every `session_meta` Codex writes states `payload.id`, so a header
+    // without one is a file nothing accounts for - truncated, hand-edited, or
+    // written by something that is not Codex. Its `session_id` reads like an
+    // answer and there is no second field left to check it against, and this
+    // one goes to a privacy control that reports success for whatever token it
+    // is handed. Unconfirmable is unresolvable, the same rule that refuses a
+    // blank container.
+    //
+    // This is asked BEFORE the missing-container question, because
+    // `legacyRolloutError` diagnoses one specific file - a Codex old enough to
+    // predate `session_id` - and names the thread id it does carry as the thing
+    // not to key on. A header stating neither field is not an old Codex, so
+    // answering it with "Upgrade Codex" would send the user after a fix for a
+    // problem this file does not have. Both refuse either way; only the
+    // diagnosis differs.
+    if (only.threadId === undefined) {
+      return {
+        ok: false,
+        error: `could not resolve a session id: the only Codex rollout recording cwd ${args.cwd} (${name}) states no thread id (payload.id), which every session_meta Codex writes carries, so the header cannot be vouched for and nothing it states about the session is evidence of the session you are in. Pass the session id explicitly: hyp session status <session-id>.`,
+      }
+    }
     if (only.sessionId === undefined) {
       return { ok: false, error: legacyRolloutError(`recording cwd ${args.cwd}`, name) }
     }
@@ -620,7 +647,12 @@ export function resolveSessionIdForCli(args) {
       error: `could not resolve a session id: no client stated one (CLAUDE_CODE_SESSION_ID, ${CODEX_THREAD_ENV}) and no Codex rollout under ${sessionsDir} records cwd ${args.cwd}. Pass the session id explicitly: hyp session status <session-id>.`,
     }
   }
-  const named = candidates.map((c) => `${c.threadId} (${path.basename(c.file)})`).join(', ')
+  // A candidate with no thread id still has to appear: it is one of the reasons
+  // the answer is ambiguous, and a user reading the list needs to see the file
+  // rather than wonder why the count exceeds the names.
+  const named = candidates
+    .map((c) => `${c.threadId ?? 'no thread id'} (${path.basename(c.file)})`)
+    .join(', ')
   return {
     ok: false,
     error: `could not resolve a session id: ${candidates.length} Codex rollouts record cwd ${args.cwd} - ${named}. Pass the intended session id explicitly rather than guessing: hyp session status <session-id>.`,
@@ -666,8 +698,13 @@ function resolveFromStatedThread(scan, sessionsDir, threadId, maxScan) {
   for (const file of scan.files) {
     const meta = readRolloutMeta(file)
     if (!meta) continue
-    if (meta.threadId !== threadId) continue
-    matches.push({ ...meta, file })
+    // Unlike the cwd scan, this path is an identity test rather than a count,
+    // so a header stating no thread cannot be a match and cannot be missing
+    // one either: it names nothing to compare.
+    const found = meta.threadId
+    if (found === undefined) continue
+    if (found !== threadId) continue
+    matches.push({ ...meta, threadId: found, file })
   }
 
   if (matches.length === 0) {
@@ -712,6 +749,12 @@ function resolveFromStatedThread(scan, sessionsDir, threadId, maxScan) {
  * two coincide, so the wrong key only shows up on the subagent threads nobody
  * tests by hand.
  *
+ * Both callers establish a thread id before reaching this, which the message
+ * relies on twice: "upgrade Codex" is only the right advice for a rollout old
+ * enough to predate the field, and "its thread id is NOT that container" names
+ * a value the header has to be carrying. A header stating neither field is a
+ * different failure and gets its own refusal.
+ *
  * @param {string} which  how the rollout was selected, for the message
  * @param {string} names  rollout basename(s) the refusal is about
  * @returns {string}
@@ -801,7 +844,19 @@ function describeAge(ms) {
 
 /**
  * The thread id, session container and cwd a rollout's `session_meta` header
- * states, or `undefined` when the file states no thread at all.
+ * states, or `undefined` when the file's first line is not that header at all.
+ *
+ * **`undefined` means "this file establishes nothing about any session", and
+ * nothing else.** It used to also swallow a header that states a `cwd` and a
+ * container but no `payload.id`, which made such a rollout invisible to the
+ * caller rather than unresolvable to it. The cwd scan counts its candidates to
+ * decide whether the match is unique, so a discarded match did not merely fail
+ * to resolve, it removed the evidence that the survivor was not alone: two
+ * rollouts recording one cwd resolved confidently to whichever of them happened
+ * to carry a thread id. That is the artefact-of-the-bound failure LLP 0067
+ * §cli-session-id refuses on, reached by discard instead of by truncation.
+ * Judging a header is the caller's job, so a header is now returned whenever it
+ * is one and the callers refuse on what they need.
  *
  * The read itself is `readRolloutSessionMeta`, shared with `@hypaware/codex`'s
  * live cwd resolver, which asks this exact line the same question for the
@@ -832,13 +887,13 @@ function describeAge(ms) {
  * rollout whose header records no usable `cwd` still answers it.
  *
  * @param {string} file
- * @returns {{ threadId: string, sessionId: string | undefined, cwd: string | undefined } | undefined}
+ * @returns {{ threadId: string | undefined, sessionId: string | undefined, cwd: string | undefined } | undefined}
  * @ref LLP 0067#cli-session-id [implements]: an absent or unusable session_id is
  * unresolvable, never the back-filled thread id
  */
 function readRolloutMeta(file) {
   const meta = readRolloutSessionMeta(file)
-  if (meta?.threadId === undefined) return undefined
+  if (!meta) return undefined
   return { threadId: meta.threadId, sessionId: meta.sessionId, cwd: meta.cwd }
 }
 

llp/0067-session-opt-out.design.md

@@ -451,6 +451,46 @@ the `hypaware-privacy` skill body does. Guessing here would opt out the wrong
 session while telling the user they are covered: the same fail-open shape this
 change exists to remove.
 
+**Recording the cwd is the whole of what makes a rollout a candidate.** The
+count *is* the uniqueness claim, so nothing else may filter the list before it
+is taken: a `session_meta` header dropped for a field this path does not resolve
+on takes the ambiguity with it and leaves a confident answer behind. Whether a
+candidate is *usable* (a live enough mtime, a readable container, a header the
+resolver can vouch for) is decided after the count, on the one candidate. This
+was wrong in the first implementation, which discarded a header stating no
+`payload.id` inside the reader wrapper: two rollouts recording one cwd resolved
+at `ok: true` to whichever of them carried a thread id, with no disclosure that
+a rival had been thrown away
+([issue #499](https://github.com/hyparam/hypaware/issues/499) §1). That is the
+artefact failure of the paragraph below, reached by a discard rather than by the
+bound.
+
+A lone such header still **refuses** rather than resolving the container it
+states: every `session_meta` Codex writes carries `payload.id`, so a header
+without one is a file nothing accounts for, there is no second field left to
+check its `session_id` against, and the control route reports `ignored: true`
+for whatever token it is handed. Counting an unvouchable header is not trusting
+it, exactly as the missing-`session_id` refusal below reads the raw line without
+trusting it.
+
+That refusal is the one asked **second**. A header stating neither field is not
+a pre-field Codex - the back-fill that rule exists to defeat needs an `id` to
+back-fill *from* - so answering it with "upgrade Codex" would name a fix for a
+problem the file does not have. Both are fail-closed, so only the diagnosis is
+at stake, and the diagnosis is the whole value of a refusal a user has to act
+on.
+
+**The rule holds for this verb only, and the Codex `hypaware-privacy` skill body
+is now the exception.** Its Step 1 disk scan still drops an id-less header
+before its own count (`not payload.get('id')`), so on the two-rollouts fixture
+above the verb refuses while the script resolves and POSTs the survivor. That is
+the divergence round 4 of [#456](https://github.com/hyparam/hypaware/pull/456)
+predicted, live now rather than hypothetical, and it is a fail-open on the
+script path alone. It is left standing because
+[#435](https://github.com/hyparam/hypaware/issues/435) retires that script onto
+this verb rather than repairing it, so until then the skill body must not be
+read as a second implementation of the rule above.
+
 The rollout walk is bounded so a very large history cannot turn a privacy check
 into a long directory scan. **A truncated walk also refuses**: "exactly one cwd
 match" over a partial listing is an artefact of the bound, not a fact, since

test/plugins/ai-gateway-session-status.test.js

@@ -362,6 +362,122 @@ test('refuses (never guesses newest) when several Codex rollouts match the cwd',
   assert.match(out.ok ? '' : out.error, /codex-bbb/)
 })
 
+test('a cwd match with NO thread id still makes the answer ambiguous: it is not discarded before the count', () => {
+  // Issue #499 §1, round 4's `x1` fixture, executed rather than reasoned about.
+  //
+  // The cwd path's whole claim is uniqueness: "exactly one rollout records this
+  // cwd, so it is the session I am in". `readRolloutMeta` used to return
+  // `undefined` for a `session_meta` header stating no `payload.id`, which the
+  // scan loop reads as "not a rollout" - so a header that demonstrably recorded
+  // this cwd never reached the candidate list. Two sessions had run here, and
+  // the resolver answered with whichever of them happened to carry a thread id,
+  // at `ok: true`, with no disclosure that it had thrown a rival away. That is
+  // the artefact-of-the-bound failure LLP 0067 refuses on for a truncated scan,
+  // arriving through a discard the bound never touched, and it lands on the
+  // privacy verb: `hyp session ignore` opts out one of two indistinguishable
+  // sessions and prints success.
+  //
+  // The thread id is not what this path resolves (it returns the container, and
+  // carries the thread only as provenance), so requiring one to be *counted*
+  // was never load-bearing - only to be *resolved*, which the next test pins.
+  const home = tempCodexHome([
+    { file: 'rollout-2026-01-01-aaa.jsonl', noThread: true, sessionId: 'container-noid', cwd: '/repo/here' },
+    { file: 'rollout-2026-01-02-bbb.jsonl', id: 'thread-bbb', sessionId: 'container-bbb', cwd: '/repo/here' },
+  ])
+  const out = resolveSessionIdForCli({ env: { CODEX_HOME: home }, cwd: '/repo/here' })
+  assert.equal(out.ok, false, 'two rollouts record this cwd, so neither is the unique match')
+  assert.equal(
+    (out.ok ? '' : out.error).includes('container-bbb'),
+    false,
+    'the survivor must not be resolved just because its rival lacked a thread id'
+  )
+  assert.match(out.ok ? '' : out.error, /2 Codex rollouts record cwd/)
+  assert.match(
+    out.ok ? '' : out.error,
+    /rollout-2026-01-01-aaa\.jsonl/,
+    'the id-less candidate is a reason for the refusal, so it has to be named'
+  )
+  assert.match(out.ok ? '' : out.error, /rollout-2026-01-02-bbb\.jsonl/)
+})
+
+test('a LONE cwd match with no thread id refuses rather than resolving the container it states', () => {
+  // Issue #499 §1, round 4's `x2` fixture: the other half of the trade the
+  // issue put on record. Counting an id-less header (above) must not turn into
+  // resolving one. Every `session_meta` Codex writes states `payload.id`, so a
+  // header without one is a file nothing accounts for, and its `session_id`
+  // would go to a control route that reports `ignored: true` for any token.
+  const home = tempCodexHome([
+    { file: 'rollout-2026-01-01-aaa.jsonl', noThread: true, sessionId: 'container-noid', cwd: '/repo/here' },
+  ])
+  const out = resolveSessionIdForCli({ env: { CODEX_HOME: home }, cwd: '/repo/here' })
+  assert.equal(out.ok, false, 'an unvouchable header is unresolvable, not an answer')
+  assert.equal(
+    (out.ok ? '' : out.error).includes('container-noid'),
+    false,
+    'the container off an unvouchable header must not be offered as the id'
+  )
+  assert.match(out.ok ? '' : out.error, /payload\.id/, 'say which field is missing')
+  assert.match(
+    out.ok ? '' : out.error,
+    /rollout-2026-01-01-aaa\.jsonl/,
+    'the refusal names the file, unlike the old "no rollout records cwd" message which denied it existed'
+  )
+  assert.match(out.ok ? '' : out.error, /explicitly/, 'point at the escape hatch')
+})
+
+test('a header stating NEITHER field is not diagnosed as an old Codex', () => {
+  // Newly reachable once an id-less header survives to the single-candidate
+  // checks: before that it was dropped inside the reader and never got a
+  // diagnosis at all. Both refusals here are fail-closed, so this is about
+  // which one the user is sent to act on. `legacyRolloutError` describes one
+  // specific file - a Codex predating `session_meta.session_id` - and tells the
+  // user to upgrade; it also asserts "its thread id is NOT that container",
+  // naming a value this header does not carry. Answering a file nothing
+  // accounts for with "Upgrade Codex" sends the user after a fix for a problem
+  // it does not have.
+  const home = tempCodexHome([
+    { file: 'rollout-2026-01-01-aaa.jsonl', noThread: true, legacy: true, cwd: '/repo/here' },
+  ])
+  const out = resolveSessionIdForCli({ env: { CODEX_HOME: home }, cwd: '/repo/here' })
+  assert.equal(out.ok, false, 'still unresolvable: neither field is readable')
+  assert.match(out.ok ? '' : out.error, /payload\.id/, 'diagnose the field that is actually missing')
+  assert.equal(
+    (out.ok ? '' : out.error).includes('Upgrade Codex'),
+    false,
+    'a header with no payload.id is not an old Codex, so upgrading is not the fix'
+  )
+  assert.match(out.ok ? '' : out.error, /rollout-2026-01-01-aaa\.jsonl/)
+})
+
+test('a rollout that DOES state a thread id but no session_id is still the legacy diagnosis', () => {
+  // The other side of the ordering above: reordering the two refusals must not
+  // cost the old-Codex case its own message, which is the one that can actually
+  // be acted on.
+  const home = tempCodexHome([
+    { file: 'rollout-2026-01-06-old.jsonl', id: 'thread-legacy', legacy: true, cwd: '/repo/here' },
+  ])
+  const out = resolveSessionIdForCli({ env: { CODEX_HOME: home }, cwd: '/repo/here' })
+  assert.equal(out.ok, false)
+  assert.match(out.ok ? '' : out.error, /Upgrade Codex/)
+  assert.match(out.ok ? '' : out.error, /carries no session_id/)
+})
+
+test('a stated thread ignores an id-less rollout entirely: that path is identity, not counting', () => {
+  // The counting change above must not leak into `resolveFromStatedThread`.
+  // There a header stating no thread names nothing to compare against, so it is
+  // neither a match nor a missing one, and the stated thread still resolves.
+  const home = tempCodexHome([
+    { file: 'rollout-2026-01-01-aaa.jsonl', noThread: true, sessionId: 'container-noid', cwd: '/repo/here' },
+    { file: 'rollout-2026-01-02-bbb.jsonl', id: 'thread-live', sessionId: 'container-live', cwd: '/repo/here' },
+  ])
+  const out = resolveSessionIdForCli({
+    env: { CODEX_HOME: home, CODEX_THREAD_ID: 'thread-live' },
+    cwd: '/repo/here',
+  })
+  assert.equal(out.ok && out.sessionId, 'container-live')
+  assert.equal(out.ok && out.source, 'codex_env_rollout')
+})
+
 test('refuses when no Codex rollout matches the cwd', () => {
   const home = tempCodexHome([
     { file: 'rollout-2026-01-01-aaa.jsonl', id: 'codex-aaa', cwd: '/repo/elsewhere' },
@@ -1019,15 +1135,20 @@ function dropContext(ignored) {
  * `type` overrides the first line's envelope type, so a test can present a
  * record that carries `id`/`session_id`/`cwd` but is not the session header.
  *
- * @param {{ file: string, id: string, sessionId?: unknown, legacy?: boolean, cwd: string, ageMs?: number, type?: string }[]} rollouts
+ * `noThread: true` omits `payload.id` - the mirror of `legacy`, and the shape
+ * round 4 of #456's review used to probe whether a header the resolver cannot
+ * vouch for is discarded before the cwd count or refused after it (#499 §1).
+ *
+ * @param {{ file: string, id?: string, noThread?: boolean, sessionId?: unknown, legacy?: boolean, cwd: string, ageMs?: number, type?: string }[]} rollouts
  */
 function tempCodexHome(rollouts) {
   const home = fs.mkdtempSync(path.join(os.tmpdir(), 'hyp-codex-home-'))
   const dir = path.join(home, 'sessions', '2026', '01')
   fs.mkdirSync(dir, { recursive: true })
   for (const r of rollouts) {
     /** @type {Record<string, unknown>} */
-    const payload = { id: r.id, cwd: r.cwd }
+    const payload = { cwd: r.cwd }
+    if (!r.noThread) payload.id = r.id
     // `in` rather than `??` so an explicit null survives as a null: a field
     // present with an unusable value is a distinct case from an absent one.
     if (!r.legacy) payload.session_id = 'sessionId' in r ? r.sessionId : r.id