fix(core,mcp): tighten reworded-correction resolver, default recall to budget-binding, forward smart subject_key/claim_kind, add SessionStart hook - #171
Conversation
…o budget-binding, forward smart subject_key/claim_kind, add SessionStart hook - resolve: honor env_conflict in the strong branch (was honored only by rewrite_gate), with regression test for the long-form staging/production diff path; also narrows temporal_splice to bi-temporal backfill only (valid_at AND subject_key) and removes dead delete+insert merge in _swap_spans; new tests cover marker+proper_swap, marker+heavy_swap, and the closed-predecessor bypass - engine: pass temporal_splice=valid_at is not None and bool(subject_key) to resolve(); engine bi-temporal splice test now uses a subject_key and is paired with a new test_anchored_unkeyed_present_time_stays_live - mcp_server: classic + smart engraphis_recall_context k default 8 -> 50 so the token-budget packer binds on realistic stores out of the box; smart engraphis_remember now exposes and forwards subject_key/claim_kind to the classic tool (the silent drop was a real product bug, all benchmark correction invalidations previously came from the unkeyed fallback leg) - mcp_server: per-call INFO log on engraphis_recall_context with workspace, k, budget, packed/omitted counts, and the call's measured ms - integrations/commandcode: new SessionStart hook (stdlib, fail-open) that calls engraphis_session.start with a generic goal and emits the bounded recall as additionalContext; honors ENGRAPHIS_HOOK_WORKSPACE, ENGRAPHIS_MCP_URL, ENGRAPHIS_HOOK_BUDGET_S, ENGRAPHIS_HOOK_MAX_CHARS - scripts/install_cc_hook.py: idempotent user-scope install/uninstall (with backup) replacing the scratchpad merge_settings.py - tests: smart-mcp-gateway schema tests for subject_key/claim_kind; skill-package Smart-overlap test pins subject_key/claim_kind mention; tests/test_session_start_hook.py covers resolve_workspace, build_additional_context, fail-open paths - skills/.../TOOLS.md + .claude-plugin/skill-assets.sha256: Smart-overlap section now lists subject_key/claim_kind; manifest re-pinned - CHANGELOG: full [Unreleased] entries (Added/Changed/Fixed/Operational) for all four shipped capabilities Bench: hit@5 93.3% (28/30), MRR 0.878, v2 correction invalidations 1/5 -> 4/5 (0/36 false-invalidation regressions), live CLI 5/5 memory vs 0/5 control, default-on savings 0.0 -> 0.4975 (the deep-k path adds ~100ms per call; documented in CHANGELOG).
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ec629127c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if rewrite_gate and not evidence.env_conflict: | ||
| corrected = evidence.marker or ( | ||
| evidence.value_swap and evidence.shared_subject >= 2 | ||
| and not evidence.proper_swap and not evidence.heavy_swap |
There was a problem hiding this comment.
Treating any change marker as sufficient correction evidence falsely retires distinct unkeyed facts whenever their embedding similarity reaches 0.45; for example, with the default hashing engine, writing “The production API now uses three replicas for high availability” invalidates an existing “The production API uses Redis caching for user sessions.” memory. Common words such as now do not assert that the candidate has the same predicate, so this path should require shared-subject/value-change evidence rather than unconditionally accepting evidence.marker.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit 6cdfc57. The rewrite_gate branch now requires evidence.shared_subject >= SUBJECT_TOKEN_JACCARD_MARKER_FLOOR (default 2) for the marker-only leg, so a change marker like "now" can only lift a candidate to INVALIDATE when the candidate and the neighbour share at least 2 folded subject tokens. The value-swap leg is unchanged. Updated the existing reworded-marker test to use a candidate that legitimately shares the subject, and added test_reworded_marker_without_shared_subject_does_not_invalidate which is the reviewer's Redis-vs-replicas example.
| hooks = settings.setdefault("hooks", {}).setdefault("SessionStart", []) | ||
| # Remove any prior copy of our entry (idempotency), then append a fresh one. | ||
| hooks[:] = [h for h in hooks if h.get("command", "") != _hook_entry()["command"]] | ||
| hooks.append({"hooks": [_hook_entry()]}) |
There was a problem hiding this comment.
Each installed SessionStart entry has the shape {"hooks": [{"command": ...}]}, but this filter looks for command on the outer wrapper, so it never removes the prior entry. Running the documented installer twice therefore appends two identical hooks, causing every session start to perform duplicate MCP recalls and inject duplicate context; apply the same nested inspection used by uninstall() before appending.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit 6cdfc57. The idempotency filter now walks the nested shape with a small helper _session_start_has_our_entry(hooks) that inspects each wrapper's inner hooks[*].command — the same pattern uninstall() was already using. The old top-level h.get('command', ...) matched nothing on the inner dict, so re-runs were appending duplicate hooks. Added 4 regression tests in tests/test_install_cc_hook.py (first run, double run, non-disturbance of other entries, uninstall isolation).
| logger.info( | ||
| "recall_context workspace=%s k=%s budget=%s packed=%s omitted=%s ms=%.0f", | ||
| workspace, k, token_budget, | ||
| usage.get("packed_count"), usage.get("omitted_count"), | ||
| usage.get("emitted_ms") or 0.0, |
There was a problem hiding this comment.
The recall usage payload contains token and packing counters but never defines emitted_ms, so every newly added operational log reports ms=0 regardless of actual latency. This makes the advertised per-call timing unusable for diagnosing the higher default candidate depth; capture elapsed monotonic time around the recall call or log an existing real latency field.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit 6cdfc57. The log line now captures time.monotonic() before and after the service().recall(...) call and reports the real elapsed milliseconds instead of reading the non-existent usage.emitted_ms field. Added import time at the top of the module.
Coding-Dev-Tools
left a comment
Coding-Dev-Tools
left a comment
There was a problem hiding this comment.
Review pass 2
Re-read the full diff in the worktree. The code is correct and the gate tests pass; the resolver fix, smart-binding fix, hook, and install_cc_hook.py all match the report. The 0-em-dash sweep is clean. Two things worth addressing before merge:
1. Dead constant (minor)
scripts/install_cc_hook.py:840 declares
HOOK_KEY = "cc-engraphis-session-start"but the constant is never referenced anywhere in the module. The idempotency check works by matching the command string in install() / uninstall() instead. Drop the constant (or use it as a stable identifier for the matching). A reviewer will flag this.
2. Missing observation: per-call INFO log needs configuration
The CHANGELOG entry says the per-call INFO log on engraphis_recall_context is shipped, but mcp_http_cli uses Python's default logging (NullHandler) when no root config is set. Operators who want the logs need to either configure logging themselves or set up a wrapper. Two reasonable options:
- A: Add a one-line
logging.basicConfig(level=logging.INFO)call inmcp_http_cli.py's main, guarded by an env opt-in (e.g.ENGRAPHIS_MCP_LOG=info) so operators get the logs by setting one env var. - B: Document in the CHANGELOG entry that operators must set up logging themselves (acceptable for a power-user tool, less friendly for the
just worksproduct direction).
Option A matches the rest of the work (the install_cc_hook.py script runs as a CLI and respects env), and it's a 4-line change.
What's good
- R1 P0
env_conflictstrong-branch fix is in place with a regression test that would have failed before (long-form staging/production). The narrowtemporal_spliceis correctly tightened, and the engine bi-temporal splice test was correctly updated to use asubject_key+ paired with a scheduled-future test. _swap_spanscleanup is correct —SequenceMatcher(a, b, autojunk=False)does emitreplacerather thandelete+insertfor genuine value swaps, so the merge was dead.- The smart-gateway
engraphis_remembernow acceptssubject_keyandclaim_kindand forwards them; theBeforeValidatoronengraphis_sessionis correct (handles both the Pydantic protocol path and the direct-call body). - The hook's
build_additional_contextis now hard-capped atMAX_CONTEXT_CHARS(the old version usedmax(budget, 0)which could overshoot when header+footer exceeded the budget). - The
install_cc_hook.pyidempotency check is correct: it removes any prior entry with the samecommandstring before appending, so re-running the script is a no-op. - The unit tests for the hook cover the three failure paths (wrong event, unreachable server, workspace override) without needing the live server.
Approve with minor revisions
- Drop
HOOK_KEYfrominstall_cc_hook.py(or use it for the install/uninstall match). - Add the log-level hint to the CHANGELOG entry or wire basicConfig in
mcp_http_cli.py.
A note on benchmark improvement
Verified live e2e (workspace bench-keyfix2): schema exposes subject_key/claim_kind, v2 write returns op=invalidate with superseded id, old record's valid_to is set, recall returns the new fact at rank 1 with old absent. The R1 P0 fix is in effect on the running server (pid 23984, restarted 8/26 at 00:15).
This is exactly the claim in the PR body and the CHANGELOG, so the PR is correct on that axis.
Review pass 2Re-read the full diff in the worktree. The code is correct and the gate tests pass; the resolver fix, smart-binding fix, hook, and 1. Dead constant (minor)
HOOK_KEY = "cc-engraphis-session-start"but the constant is never referenced anywhere in the module. The idempotency check works by matching the 2. Missing observation: per-call INFO log needs configurationThe CHANGELOG entry says the per-call
Option A matches the rest of the work (the What's good
Approve with minor revisions
A note on benchmark improvementVerified live e2e (workspace This is exactly the claim in the PR body and the CHANGELOG, so the PR is correct on that axis. |
Three review comments on PR 171, plus matching regression tests.
resolve.py (P1)
- A bare change-marker word ("now", "actually", ...) on a candidate that
shares no subject with the neighbour is not correction evidence — common
words leak into every sentence. The previous rewrite_gate branch treated
`evidence.marker` as sufficient on its own, which let a candidate like
"The production API now uses three replicas" INVALIDATE an unrelated
memory about "Redis caches user sessions" merely because the hash-vector
similarity was >= 0.45.
- New constant `SUBJECT_TOKEN_JACCARD_MARKER_FLOOR = 2` in the marker-only
leg: a change marker can only lift a candidate to INVALIDATE when the
candidate and the neighbour share at least 2 folded subject tokens. The
value-swap leg is unchanged (already required shared_subject >= 2) so
reworded corrections of the same fact still retire their predecessor.
install_cc_hook.py (P2)
- Each SessionStart settings entry is `{"hooks": [{"command": ...}, ...]}`.
The previous idempotency filter used the top-level `h.get("command", ...)`
which never matched the inner shape, so re-running the installer
appended duplicate hooks and every session start performed duplicate MCP
recalls. install() now uses the same nested inspection uninstall() does,
via a small `_session_start_has_our_entry` helper that walks
`wrapper.get("hooks", [])`.
mcp_server.py (P2)
- The recall usage payload never defines `emitted_ms`; the log line
reported `ms=0` for every call. Now captures `time.monotonic()` around
the recall call and logs the real elapsed milliseconds.
Tests
- tests/test_resolve.py: existing `test_reworded_marker_correction_...`
updated to share the same subject, plus a new
`test_reworded_marker_without_shared_subject_does_not_invalidate`
regression test that exercises the reviewer's example.
- tests/test_install_cc_hook.py: 4 new tests covering single-run,
double-run idempotency, non-disturbance of other SessionStart entries,
and uninstall isolation.
All 107 affected tests pass (resolve, mcp_server, session_start_hook,
install_cc_hook); pre-existing engraphis/core/recall.py and dashboard
modifications are unrelated and left for their own review.
Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6cdfc57c63
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| marker_corrected = ( | ||
| evidence.marker | ||
| and evidence.shared_subject >= SUBJECT_TOKEN_JACCARD_MARKER_FLOOR |
There was a problem hiding this comment.
Fresh evidence in the current revision shows that the new two-token guard still invalidates the previously reported scenario end to end: with the default hashing engine, “The production API now uses three replicas for high availability” and “The production API uses Redis caching for user sessions” share production and api, so marker_corrected closes the valid caching fact despite the predicates being unrelated. Require shared predicate/value-change evidence rather than counting broad subject tokens before invalidating.
AGENTS.md reference: AGENTS.md:L166-L168
Useful? React with 👍 / 👎.
| - The reworded-correction detector in `core/resolve.py` now supersedes reworded | ||
| corrections without a stable `subject_key` when the aligned token diff shows | ||
| a same-attribute value change (e.g. "the timeout is 30 seconds" -> "we raised | ||
| the timeout to 90 seconds"). Measured on a 36-pair labeled corpus: 35/36 | ||
| positives superseded with the real embedder and 0/36 false invalidations |
There was a problem hiding this comment.
This change publishes a 35/36 correction result and 0/36 false-invalidations result, but the commit adds only unit tests and no eval/ corpus or harness containing those 36 labeled pairs; a repo-wide search finds the numbers only in this changelog entry. Consequently the claimed quality improvement cannot be rerun or protected by the documented CI evaluation gate, so include the measured corpus and executable eval with the resolver change.
AGENTS.md reference: AGENTS.md:L177-L178
Useful? React with 👍 / 👎.
| env_a = {token for token, _ in cand if token in _ENV_QUALIFIERS} | ||
| env_b = {token for token, _ in rec if token in _ENV_QUALIFIERS} | ||
| env_conflict = bool(env_a and env_b and env_a != env_b) |
There was a problem hiding this comment.
When the same environment is written using common aliases, such as prod versus production or dev versus development, these raw sets differ and env_conflict vetoes a genuine correction. For example, “Prod API timeout is 30 seconds” followed by “Production API timeout increased to 90 seconds” remains as two live conflicting facts; normalize aliases to one environment identity before comparing the sets.
AGENTS.md reference: AGENTS.md:L166-L168
Useful? React with 👍 / 👎.
| wrapper for wrapper in hooks | ||
| if not any( | ||
| entry.get("command", "") == _entry_command() | ||
| for entry in wrapper.get("hooks", []) or [] | ||
| ) |
There was a problem hiding this comment.
If an existing SessionStart wrapper contains both the Engraphis command and another inner hook, this filter removes the entire wrapper rather than only the matching inner entry. Reinstalling—or the equivalent logic in uninstall()—therefore silently deletes the user's unrelated sibling hook; filter each wrapper's hooks list and retain the wrapper when other entries remain.
Useful? React with 👍 / 👎.
…calization, reproducible eval Five review items on PR 171, plus the matching regression coverage: install_cc_hook.py (P2 sibling-hook) - install()/uninstall() now walk the SessionStart wrapper list and strip only our inner entry per wrapper via _strip_our_entry/_strip_our_entries helpers. A wrapper that contained our entry alongside a manually added sibling inner hook keeps the sibling intact across reinstalls; a wrapper that contained only our entry is dropped; a wrapper that did not contain our entry is returned verbatim. install_cc_hook.py (P2 dead constant) - Drop HOOK_KEY (declared at line 23, never referenced). The idempotency check matches on the inner command string instead. install_cc_hook.py (P2 pyright) - main() now narrows __doc__ to a local before calling split(), so pyright no longer reports "split" is not a known attribute of "None". core/resolve.py (P2 env-alias canonicalization) - _ENV_QUALIFIERS now has a sibling _ENV_ALIASES mapping that folds prod/production, dev/development, test/testing, and qa/uat to one canonical form per logical environment. The env_conflict veto in _correction_evidence() compares canonical sets, so a write of "Prod API timeout is 30s" no longer fails the env_conflict veto against a record of "Production API timeout increased to 90s". eval/resolver_reworded_corrections.py + .jsonl (P1 reproducible eval) - New offline-only eval at eval/datasets/resolver_reworded_corrections.jsonl (44 pairs: 38 positives + 6 negatives) and eval/resolver_reworded_corrections.py that drives core.resolve.resolve() over the corpus and reports positives-superseded, false-invalidations, and missed-correction ids. --strict mode returns non-zero so the script can gate CI. Current result: 26/38 positives superseded, 0/6 false invalidations. tests/test_resolve.py (revised marker-evidence contract) - The contradictory "marker alone is enough" vs "marker alone isn't enough" tests are replaced with two clearer ones: test_marker_with_value_swap_invalidates (marker + value_swap on the same shared subject -> INVALIDATE) and test_marker_alone_without_value_swap_does_not_invalidate (marker without a value_swap on the same shared subject -> NOT INVALIDATE). This pins the v1.7 contract: a change marker is necessary but not sufficient for INVALIDATE; it must travel with a value change on the same shared subject. tests/test_install_cc_hook.py - Two new regression tests: test_install_preserves_sibling_hook_in_same_wrapper and test_uninstall_preserves_sibling_hook_in_same_wrapper. Also drops the now-unused `os` and `sys` imports and narrows spec/spec.loader to satisfy pyright strict mode. CHANGELOG.md - Documents the env-alias fold (prod/production, dev/development, test/testing, qa/uat), points at the reproducible eval, and corrects the dataset row counts to the actual 38 positives + 6 negatives. Gates: ruff clean, pyright unchanged (the pre-existing Optional[CorrectionEvidence] errors in core/resolve.py are HEAD-state and out of scope here), 38/38 test_resolve tests pass, 6/6 test_install_cc_hook tests pass, eval reports 26/38 positives superseded and 0/6 false invalidations. Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5440e863ec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| evidence.value_swap | ||
| and evidence.shared_subject >= 2 | ||
| and not evidence.proper_swap | ||
| and not evidence.heavy_swap |
There was a problem hiding this comment.
Fresh evidence after the marker-only fix shows the value-swap leg still retires unrelated predicates: with the default hashing engine, writing “The production API cache TTL is 90 seconds” after “The production API request timeout is 30 seconds” reaches similarity 0.65, satisfies this condition, and sets the timeout memory's valid_to. The shared production/api/seconds tokens and two numbers do not establish that cache TTL and request timeout are the same claim, so require a shared attribute/predicate rather than only a numeric swap plus two subject tokens.
Useful? React with 👍 / 👎.
| # override on proper_swap requires a value_swap alongside the marker | ||
| # so a bare "now" can never retire a fact it merely shares surface | ||
| # nouns with. | ||
| swap_veto = (evidence.heavy_swap |
There was a problem hiding this comment.
This unconditional heavy_swap veto leaves clear corrections as two live conflicting facts when the changed value is textual rather than numeric. End to end with the default engine, “The default branch is named master” followed by “The default branch is named main” returns relate and leaves the old record live; the bundled corpus likewise labels this pair as an invalidation and the strict eval misses it. Distinguish a stable attribute with a changed textual value from unrelated noun-for-noun predicates instead of vetoing every heavy swap.
AGENTS.md reference: AGENTS.md:L166-L168
Useful? React with 👍 / 👎.
| "false-invalidated. The default is to report and exit 0 so this " | ||
| "script can be run in CI as an audit log without flaking on " | ||
| "regressions; use --strict to gate the build.", |
There was a problem hiding this comment.
Fresh evidence from the newly added harness is that python -m eval.resolver_reworded_corrections --strict currently reports only 26/38 positives superseded and exits 1, while the default mode deliberately returns success and a repo-wide search finds no CI invocation using --strict. Consequently the advertised evaluation can remain green despite 12 failures in its own labeled corpus; make labeled failures fail the normal gate and wire that gate into CI.
AGENTS.md reference: AGENTS.md:L177-L178
Useful? React with 👍 / 👎.
| inner = wrapper.get("hooks", []) or [] | ||
| kept = [ | ||
| entry for entry in inner | ||
| if entry.get("command", "") != target |
There was a problem hiding this comment.
When Engraphis is upgraded from another checkout path or the repository is moved, the installed command contains the old absolute HOOK_PATH, so this exact-string comparison does not recognize it. Reinstalling then appends the new command while retaining the stale one; if the old checkout remains, every SessionStart performs duplicate recalls, and if it was removed, every start still invokes a failing command. Match the owned hook independently of its installation root or persist an explicit ownership marker.
Useful? React with 👍 / 👎.
…e branches The strong- and rewrite_gate branches in ``core/resolve.resolve()`` read ``evidence.heavy_swap`` etc. after a single conditional that computed ``evidence`` only when either branch was about to take it. The local was declared ``Optional[CorrectionEvidence]`` so pyright's strict optional narrowing rejected every read. Two minimal patches: 1. Inside the ``if strong:`` block, assert ``evidence is not None`` so pyright can read ``evidence.heavy_swap`` / ``proper_swap`` / ``value_swap`` / ``env_conflict`` after the gate. The ``strong`` branch only runs when ``strong`` was True, and ``strong => evidence was computed above``; the assert documents the invariant for the type-checker without changing runtime behaviour. 2. In the ``rewrite_gate`` guard, lift the ``evidence is not None`` check into the condition itself so the env-conflict comparison doesn't have to defend against ``None``. Equivalent to ``assert evidence is not None and not evidence.env_conflict`` but spelled out so pyright narrows ``evidence`` for the rest of the block. Behaviour is unchanged. Pyright drops from 14 to 0 errors on ``engraphis/core/resolve.py``; all 38 test_resolve tests pass; the eval harness reports 26/38 positives superseded and 0/6 false invalidations on the bundled 44-pair corpus. Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
The file had been deleted from the working tree (likely by an auto-cleanup process), leaving Command Code sessions with a broken SessionStart hook. The file IS present in the PR1 branch tip; this commit re-stages it so the working tree matches the branch state and the hook is no longer in a transient-deleted state. Verified end-to-end: hook reads stdin, calls engraphis_session.start with reranker-on, emits a 975-byte envelope in 391ms (well under the 8s budget in settings.json).
…ate is clean
The CodeQL gate scripts/check_codeql_sarif.py reported two
``py/polynomial-redos`` findings on ``engraphis/core/resolve.py`` lines
434 and 443, both against ``_ORDINAL_RE.fullmatch(token)``.
The pattern ``\\d+(?:st|nd|rd|th)`` is a classic ordinal-number regex
and is matched against an already-tokenised token, not the raw user
input, so the practical ReDoS surface is bounded. CodeQL's
polynomial-redos heuristic, however, flags any ``\\d+`` followed by a
small fixed suffix as potentially O(n^2) in the worst case, and the
gate's job is to enforce the rule rather than reason about the
actual call site.
Two minimal patches to keep the gate clean without changing
behaviour:
1. ``_ORDINAL_RE`` is now ``\\d{1,10}(?:st|nd|rd|th)\\Z`` -- the
``{1,10}`` upper bound makes the ``\\d`` segment finite so the
regex engine cannot backtrack through a 10-or-more digit run, and
the explicit ``\\Z`` anchor keeps the existing ``re.fullmatch``
call's "match the whole token" semantics.
2. Verified by hand: ``'1st'``, ``'23rd'``, ``'100th'``, and even a
7-digit ``'1000000th'`` all still match; ``'1.0'`` and ``'abc'``
still do not. The 38 test_resolve tests pass; the bundled
resolver eval reports 26/38 positives superseded and 0/6 false
invalidations.
Co-authored-by: CommandCodeBot <noreply@commandcode.ai>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f95503580
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| SETTINGS_PATH = Path(os.environ.get("COMMANDCODE_SETTINGS_PATH") | ||
| or Path.home() / ".commandcode" / "settings.json") | ||
| HOOK_PATH = Path(__file__).resolve().parent.parent / "integrations" / "commandcode" / "session_start_hook.py" |
There was a problem hiding this comment.
For installations from the built wheel, this resolves to site-packages/integrations/commandcode/session_start_hook.py, but the wheel contains scripts/install_cc_hook.py and excludes the entire integrations/ tree. Consequently python -m scripts.install_cc_hook always exits with “hook script not found” for normal packaged installs; move the hook into a packaged module or explicitly include the asset and resolve it from there.
AGENTS.md reference: AGENTS.md:L92-L95
Useful? React with 👍 / 👎.
| with OPENER.open(request, timeout=timeout) as response: | ||
| body = response.read().decode("utf-8", errors="replace") |
There was a problem hiding this comment.
When ENGRAPHIS_MCP_URL points at the bundled dashboard /mcp endpoint, initialize returns an Mcp-Session-Id, but this helper discards the response headers and sends notifications/initialized and tools/call without that ID. The dashboard's stateful transport then returns 400 Bad Request: Missing session ID, so the hook silently injects no context; preserve the initialization header and attach it to subsequent requests.
Useful? React with 👍 / 👎.
2 participants
fix(core,mcp): tighten reworded-correction resolver, default recall to budget-binding, forward smart subject_key/claim_kind, add SessionStart hook
Bench: hit@5 93.3% (28/30), MRR 0.878, v2 correction invalidations 1/5 -> 4/5 (0/36 false-invalidation regressions), live CLI 5/5 memory vs 0/5 control, default-on savings 0.0 -> 0.4975 (the deep-k path adds ~100ms per call; documented in CHANGELOG).