Fix #1631: hypaware-privacy's Codex copy still falls back to the stale 127.0.0.1:8787 gateway default that LLP 0212 recorded as shipped and #1620 fixed in the Claude copy only - #1632
Conversation
…e 127.0.0.1:8787 default Correct the Codex copy's Step 1 shell fallback to DEFAULT_GATEWAY_ENDPOINT (http://127.0.0.1:18521), and widen the drift guard so both copies are pinned. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
# Conflicts: # test/fixtures/skill-host-divergence.json
Review round 1 -
|
| ref | hypaware-privacy.hash |
|---|---|
0bfcb698 (merge base) |
4e08acf6ce44c72e |
a9da165a (branch side) |
e612bd514f92238a |
daa438a7 (master side, #1630) |
5f570adb6b0d663b |
b0b07a44 (merge) |
dc25aaad98682fb1 |
Regenerating rather than hand-picking a side is the only correct resolution for a tree carrying both edits, and I confirmed it is not a typed value: node scripts/record-skill-host-divergence.js on the merged tree leaves git status clean, so the committed hash is byte-for-byte what the recorder produces. Counts stay 31 / 88, which is right - #1630 replaced a claude-only line with another claude-only line, #1632 replaced a codex-only line with another codex-only line, so neither moved a line across the divide. The recorder is idempotent.
The guard file did not conflict, and both guards survive intact. The origin/master to HEAD diff of test/plugins/ai-gateway-session-ignore-receipt.test.js is a single hunk at @@ -291,21 +291,40 @@: the standalone claude-only default test is replaced by the FALLBACK_DEFAULT map plus its loop. #1630's FALLBACK_SCOPE map (lines 179-182) and its per-copy assert.match(step1, FALLBACK_SCOPE[rel]) (line 288) do not appear in the diff at all, i.e. they are carried through untouched. Neither guard is weakened; the file now holds three per-copy loops (SKILLS x reply validation, x CLI-verb-first + scoping, x gateway default) and one deliberate claude-only test.
The Claude copy is untouched by this branch. git diff origin/master HEAD -- .../claude/skills/hypaware-privacy/SKILL.md is empty. Byte-identical.
1. Does the guard bite in every direction? Yes - re-verified, 6/6, plus a seventh
Each mutation applied to the merged tree, then node --test over the guard file and test/plugins/skill-host-parity.test.js. Baseline is 16 pass / 0 fail.
| # | Mutation | Result |
|---|---|---|
| M1 | 8787 restored in the Codex copy |
not ok codex default guard + host-divergence |
| M2 | 8787 restored in the Claude copy |
2 fail |
| M3 | #1630's stale scoping sentence restored in the Claude copy | not ok claude CLI-verb guard + host-divergence |
| M4 | Codex copy given the Claude scoping wording | 2 fail |
| M5 | Claude copy given the Codex default expansion (BASE:-) |
2 fail |
| M6 | Codex copy given the Claude default expansion (ANTHROPIC_BASE_URL:-) |
not ok codex default guard + host-divergence |
| M7 | DEFAULT_GATEWAY_ENDPOINT changed to :19999 |
both copies fail (not ok 9 and not ok 10) |
M7 is the one that matters most and it is the one the old claude-only test could not have produced. No mutation was silently absorbed; the tree was verified clean after each revert.
Worth noting the substring hazard did not bite: ANTHROPIC_BASE_URL:- does not contain BASE:- (it ends BASE_URL:-), so the codex assertion cannot be accidentally satisfied by the claude expansion, or vice versa. M5/M6 prove that empirically.
2. Is the regenerated fixture a live guard? Yes
Confirmed directly: M1 fails hypaware-privacy: host-specific content matches the recorded surface, not only the new codex case. Every one of M1-M6 trips it, because each edits a line inside the recorded divergence set. And idempotence is proven above (re-run leaves the tree clean), so the fixture is not a rubber stamp that only agrees with whatever was typed.
Caveat worth stating plainly, because it is why this issue existed at all: the divergence fixture records that host-specific content changed and asks for a deliberate re-record. It never asserts the content is correct. That is exactly how the stale 8787 survived inside the recorded set for as long as it did. The semantic pin in the guard file is the thing doing the work here; the fixture is a second, weaker net.
3. Claude copy untouched? Yes - verified above, empty diff against origin/master.
4. Is 18521 right, and read from the constant? Yes to both
DEFAULT_GATEWAY_ENDPOINT = http://127.0.0.1:18521 (src/core/config/gateway_endpoint.js:16), mirroring the plugin's DEFAULT_LISTEN = '127.0.0.1:18521' (hypaware-core/plugins-workspace/ai-gateway/src/config.js:8), the two pinned together by test/core/init-gateway-listen-default.test.js. The control route is served at the root of that listen, so ${BASE}/_hypaware/ignore/session resolves, and the new default carries no /v1 or /backend-api/codex suffix for the strip lines below it to chew on.
The guard imports the constant (line 12) and interpolates it into both FALLBACK_DEFAULT entries, so it pins the skill to the constant rather than to a typed literal. M7 is the positive proof: change the constant and both copies fail, which is precisely the property the claude-only version lacked and the property #1631 asked for.
5. Remaining Claude/Codex asymmetry in the guard's coverage
Audited every pin over the two copies. Two asymmetries the PR body already names are real and correctly deferred: the Codex copy carries no receipt-reading guidance at all (tracked as #1633, not re-filed), and it lacks the @ref LLP 0212#cli-is-the-verb annotation the Claude copy carries at line 27 (it also lacks @ref LLP 0256#cli-posts-to-both, but that one is downstream of #1633 - there is no recorders check for it to annotate).
I found one further unpinned asymmetry, running in the other direction, and filed it as #1635:
LLP 0066 §readable R9 (name both ways an opt-out lapses) is pinned for the Codex copy by
test/plugins/codex-privacy-skill-session-id.test.js:117, including adoesNotMatchthat forbids presenting the restart as the only way. The Claude copy's equivalent sentence (claude/skills/hypaware-privacy/SKILL.md:108) has no test over it. The only other--fork-sessionassertion in the suite (test/plugins/ai-gateway-session-status.test.js:271) pins the CLI'sEPHEMERAL_NOTE, a different surface. LLP 0212 §Context records "a missing caveat" as one of the two drift modes that actually shipped (#455), so this is a regression that has already happened once and is now held down on one copy only.
Out of scope here by the same reasoning the author applied to #1633: it is a different requirement with its own guard, and folding it in would grow a two-line fix into a third loop. #1635 suggests the fix follow this PR's own shape (a for (const rel of SKILLS) loop with a per-copy map in the guard file that already owns SKILLS / FALLBACK_SCOPE / FALLBACK_DEFAULT).
Nothing else: the Prefer hyp session ignore --json sentence, the three-way reply validation, drop set, and nothing more, curl --fail-with-body and the 18521 default are all pinned per copy. Both privacy skills are single-file, so skillText's SKILL.md-only read leaves no stage-file gap today.
6. Conventions
- No em dashes. 0 occurrences of U+2014 in all three changed files (checked with a codepoint scan, not a visual pass).
- No NUL bytes. 0 occurrences of U+0000 in all three.
- No new runtime dependencies.
package.jsonandpackage-lock.jsonare untouched againstorigin/master. - No semicolons introduced in the JS diff.
@refs honest.LLP 0212#cli-is-the-verb(anchor atllp/0212-...:51) sits directly above the section-3 loop and still describes it;LLP 0256#cli-posts-to-both(anchor atllp/0256-...:35) sits directly above the claude recorder-id test and still describes it. Both anchors exist; the newFALLBACK_DEFAULTblock is inserted after the 0212-annotated loop closes, so it breaks neither attachment. No LLP is needed for this change (a bug fix plus a test widening, per the "most changes need no LLP" rule), and none was added.- Reuse before adding. The new assertion lives in the file that already owns
SKILLSand the sibling per-copy maps rather than in a new file, and no other test duplicates it.
Suites
npm test: 6541 tests, 6537 pass, 3 fail - exactly the known pre-existingtest/core/hyparquet-floor-pin.test.jscases, which reproduce onorigin/master. Not a finding.node --test "test/plugins/**/*.test.js": 2325 / 2325 pass.npm run typecheck: clean.- GitHub checks on this head: all 8 green (
CI required,LLP required,cross-branch-numbers,duplicate-numbers,test (22),test (24),typecheck (22),typecheck (24)).
CPU and memory pass
No concern. The runtime change is a single literal inside shipped Markdown - no code path, no allocation, no per-record or long-running work. The test change adds one for (const rel of SKILLS) loop over two entries; it turns one readFileSync of a ~30 KB Markdown file into two, in a suite where the same two files are already read once each by three sibling loops. Bounded, one-shot, test-time only. The FALLBACK_DEFAULT map is two string entries built once at module scope from a constant, not per test. Nothing here grows with data volume or uptime, and no hot path is touched.
Non-findings considered and dismissed
FALLBACK_DEFAULT[rel]would beundefinedifSKILLSever gained a third host;text.includes(undefined)searches for the literal"undefined"and fails, so it fails closed, and the pre-existingFALLBACK_SCOPEmap immediately above has the identical shape. Consistency beats a new completeness assertion here.- The positive half pins the expansion shape (
BASE:-...), so a behaviour-preserving refactor moving the default into theOPENAI_BASE_URL:-expansion would fail it. That is the deliberate per-copy pinning the docstring explains, and M5/M6 are what make it worth having. - The positive assertion scans the whole file rather than
privacyStep1(rel). Harmless: the string exists only in Step 1, and the negative half is whole-file by design.
One stale line in the PR body (not corrected, no factual error)
The body describes #1630 as "in-flight" and quotes 6537/6540; #1630 has since landed and the merged tree is 6537/6541 (#1630 added a test). The prediction it makes was accurate, so nothing there is wrong as written and I left it alone. gh pr view 1632 --json closingIssuesReferences still returns 1631.
Verdict
Clean. The merge is correct and independently confirmed, both guards survive intact and bite in all seven directions tested, the regenerated fixture is live and idempotent, the Claude copy is byte-identical to origin/master, and the default is read from DEFAULT_GATEWAY_ENDPOINT so a change to the constant now fails both copies instead of leaving one behind. No changes pushed; head remains b0b07a44.
Edited after posting to correct one sentence of provenance. The original opening claimed two independent passes had "both come back with zero actionable findings"; the second pass had in fact not reported, and I should not have written that it had. No finding, verdict, mutation result, or command output elsewhere in this review is affected - those were all produced by the manual pass and are unchanged.
Ship risk:
|
Feature or issue
The
hypaware-privacyCodex copy's Step 1 shell fallback still defaulted its gateway base tohttp://127.0.0.1:8787(codex/skills/hypaware-privacy/SKILL.md:122), the literal Accepted LLP 0212 §Context records as a shipped defect. The real default an unpinned gateway binds isDEFAULT_GATEWAY_ENDPOINT,http://127.0.0.1:18521(src/core/config/gateway_endpoint.js:16). PR #1620 corrected this in the Claude copy and pinned it with a claude-onlydoesNotMatch(/127\.0\.0\.1:8787/), so the twin copy sat outside the guard written to forbid exactly this and kept the stale literal. Reachability is narrow (theconfig.tomlread normally wins) and it fails closed - thecurlto a dead port routes the reader to the documented stop - so this is a correctness and consistency fix, not an exposure.Solution
http://127.0.0.1:18521, read fromDEFAULT_GATEWAY_ENDPOINTrather than typed from the issue.grep -rn '127\.0\.0\.1:8787'over the tracked tree now returns no skill hit; the remaining hits are test and smoke fixtures that configurelistenexplicitly, where the value is the point.test/plugins/ai-gateway-session-ignore-receipt.test.jsfrom one claude-only test to afor (const rel of SKILLS)loop, matching the file's existing per-copy shape. ThedoesNotMatchhalf now runs over both copies; the positive half stays per-copy through aFALLBACK_DEFAULTmap (the claude script defaultsANTHROPIC_BASE_URL, the codex one defaultsBASEafterOPENAI_BASE_URLand theconfig.tomlread miss), so each copy is pinned toDEFAULT_GATEWAY_ENDPOINTby the constant and neither can drift from it silently.8787on line 122 fails the new codex case on the positive assertion, and leaving the correct default while reintroducing the literal elsewhere in the file fails it on the negative one. Re-recordedtest/fixtures/skill-host-divergence.json: the line is codex-only, so the counts are unchanged (31 / 88) and only thehypaware-privacyhash moved.npm testpasses (6537/6540, the 3hyparquet-floor-pinfailures reproduce unchanged onorigin/master);npm run typecheckis clean.Code: +0 / -0 lines
Two asymmetries between the copies are noted and deliberately not absorbed here, both larger than this issue: the Codex copy carries no receipt-reading guidance for
hyp session ignore --jsonat all (no"status": "partial"stop, nosession_id_sourcecheck, norecorderscheck - the stop list that #1615 put in the Claude copy), and it lacks the@ref LLP 0212#cli-is-the-verbannotation the Claude copy carries even though the section-3 loop already tests both copies against that decision.The fixture hash line will conflict textually with in-flight #1630, which re-records the same field; whichever lands second re-runs
node scripts/record-skill-host-divergence.js. The guard-file change is localised well below #1630'sFALLBACK_SCOPEaddition and does not overlap it.Fixes #1631