The Run Claude Code Review step passed an invalid PR ref
(owner/repo/pull/N) which gh interprets as a branch name, causing
early gh pr view failures. More importantly, the prompt omitted
--comment, so the code-review plugin only displayed findings in
terminal output and never invoked gh pr comment to post to the PR.
Switch to a full PR URL and add --comment so the plugin posts the
review during the session, which also routes around upstream bugs
anthropics/claude-code-action#1061 and #1087 where the action's
post-step capture can silently drop output on issue_comment triggers.
After U3 (FQN-keyed bindingAugmentations) lands, the FQN regression
passes under REGISTRY_PRIMARY_PHP=1 but still fails under =0. The
legacy DAG resolves receiver types via simple-name workspace lookup
and has no namespace-prefixed binding channel, so it cannot
distinguish `\App\Other\User` from a same-simple-name class
reachable via `use`. Per the established convention from commit
af9af4a9 ("parity with the legacy DAG is not a correctness
criterion when the legacy DAG itself has the same defect"), the
test registers in LEGACY_RESOLVER_PARITY_EXPECTED_FAILURES.php
rather than blocking the migration.
The other two assertions in the new describe block (class detection
and the saveLocal control) pass in both modes and stay green.
Verified:
- REGISTRY_PRIMARY_PHP=0: 174 passed, 4 skipped (3 existing + 1 new).
- REGISTRY_PRIMARY_PHP=1: 178 passed.
- C# + Python + TypeScript + Go + C resolver matrix: 794 passed.
- pickImplicitThisOverload unit tests: 5 passed.
- resolver-parity-expected-failures unit test: 3 passed.
Plan: docs/plans/2026-05-11-002-fix-php-fqn-and-overload-codex-findings-plan.md (U5)
Codex PR #1497 review, finding 2: pickImplicitThisOverload returned
candidates[0] after narrowOverloadCandidates without checking
uniqueness. When two same-name methods on the same class shared
identical arity and the call site lacked disambiguating argument-type
info, narrowing left both compatible and the resolver emitted a
high-confidence CALLS edge whose target depended on registration
order rather than a defensible resolution.
Tighten the picker:
- candidates.length === 1 -> return candidates[0] (unchanged for the
unambiguous case)
- candidates.length !== 1 (zero or multiple) -> return undefined
(call left unresolved; no edge emitted)
This mirrors pickUniqueGlobalCallable's existing pattern in the same
file.
Export pickImplicitThisOverload so a unit test can exercise it with
synthetic stubs — PHP cannot produce the multi-overload failure shape
(no method overloading in PHP), and C# integration coverage would
entangle the unit's contract with the broader C# resolver. The unit
test pins five cases: sole overload, narrowing-disambiguated, the
ambiguous multi-candidate case (the bug regression), no-match, and
no-enclosing-class.
Verified: 972/972 across PHP + C# + Python + TypeScript + Go + C
resolver suites; no regression in any language. tsc clean.
Plan: docs/plans/2026-05-11-002-fix-php-fqn-and-overload-codex-findings-plan.md (U4)
Extend populatePhpNamespaceSiblings with Step 3b: for every PHP file's
Module scope, inject a binding entry keyed by the fully-qualified
class name (`App\Models\User`) for every class-like def in the
workspace. This routes FQN-receivers like `\App\Other\User` to the
exact namespace-qualified class regardless of which simple-name `User`
the caller's `use` imports shadowed.
Why module-scope bindingAugmentations instead of mutating def.qualifiedName:
the shared QualifiedNameIndex consumes def.qualifiedName at finalize time,
but PHP class defs need to remain keyed by simple name throughout the rest
of the pipeline (MRO, method-dispatch-index, namespace-siblings step 3).
An earlier attempt to rewrite def.qualifiedName to namespace-prefixed form
cascaded into 32 unrelated test failures across receiver-binding, MRO, and
heritage. The bindingAugmentations channel is purpose-built for adding
post-finalize visibility without mutating shared semantic state, and
`findClassBindingInScope`'s scope-chain walk already consumes it via
`lookupBindingsAt` — wiring is zero-touch.
Cost: O(PHP files × class-like defs) augmentation entries. Typical PHP
project: hundreds × hundreds = bounded.
Verified locally:
- Registry-primary: 178/178 PHP tests pass (including the new FQN regression).
- Legacy DAG: 174 passed, 3 existing skips, 1 failure (the FQN test, expected
— legacy DAG has no namespace augmentation channel; U5 registers it in
LEGACY_RESOLVER_PARITY_EXPECTED_FAILURES.php).
- Cross-language: 794/794 (C#, Python, TypeScript, Go, C) pass. The PHP-only
augmentation does not touch shared resolution code.
- tsc clean.
Plan: docs/plans/2026-05-11-002-fix-php-fqn-and-overload-codex-findings-plan.md (U3)
Stop collapsing `\App\Models\User` to `User` in normalizePhpType (step 6).
Canonicalize the leading backslash off and preserve the qualified path
on TypeRef.rawName so downstream PHP receiver resolution can distinguish
the FQN target from a same-simple-name class reachable via `use`.
- Step 6 rewritten: `\App\Models\User` → `App\Models\User`,
`App\Models\User` → unchanged, `User` → unchanged.
- Final validation regex relaxed from /^\w+$/ to /^\w+(?:\\w+)*$/ to
accept qualified PHP identifiers while still rejecting empty segments,
trailing backslashes, and non-identifier characters.
- Step 5 (single-arg generic strip) already passes qualified inner types
through via its existing /^\w[\w\]*/ pattern — no change needed.
U3 will wire the qualified-name lookup path; this commit alone is a
no-op for resolution (the lookup chain still keys on simple names).
PHP suite remains green at 177 passed; the only failing test is the
intentional FQN regression from U1.
Plan: docs/plans/2026-05-11-002-fix-php-fqn-and-overload-codex-findings-plan.md (U2)
Fixture php-fqn-cross-namespace declares two `User` classes — `App\Models\User`
and `App\Other\User` — and a Service.php that imports the Models simple-name
via `use App\Models\User` but uses a fully-qualified `\App\Other\User` in a
parameter annotation. Adds three assertions:
- Both User classes are detected as Class nodes in distinct files.
- The FQN-parameter method's `$u->record()` resolves to app/Other/User.php
(currently FAILS — Codex review finding 1).
- The simple-name-parameter method's `$u->record()` resolves to
app/Models/User.php (control; still passes pre-fix).
This is the test-first gate Codex required: "Block merge until the PHP FQN
fixture fails before the fix and passes after it." Verified failing on
HEAD before U2-U3 land the fix.
Also sweeps every `toBeGreaterThanOrEqual` in php.test.ts to exact
`.toBe(N)` per DoD.md §2.7 ("avoid bounds-only assertions that mask
regressions"). Touched 10 assertions across the U2/U3 trait MRO blocks,
the namespace-aware free-call fallback block, and the new FQN block.
All previously-green tests stay green with exact counts — no latent
over-emission bugs were hidden by `>= 1`.
The ambiguous-overload finding (Codex finding 2) cannot be exercised by a
PHP integration fixture — PHP does not support method overloading, so
`model.methods.lookupAllByOwner` returns at most 1 entry per (class, name)
pair. That regression test ships as a unit test against
`pickImplicitThisOverload` in U4 once the function is exported.
Plan: docs/plans/2026-05-11-002-fix-php-fqn-and-overload-codex-findings-plan.md (U1)
* fix(augment): add CONTAINS fallback when FTS indexes unavailable
When the MCP server holds the KuzuDB write lock, the augment CLI opens
the DB read-only. FTS indexes cannot be created in read-only mode, so
searchFTSFromLbug returns ftsAvailable=false and an empty results array.
The existing early-return path silently produced no enrichment.
Add a Cypher name CONTAINS fallback that fires only when ftsAvailable is
false and BM25 produced no symbol matches. This covers the read-only DB
case (concurrent MCP server) and the first-run case (indexes not yet
built). The fallback is wrapped in .catch(() => []) and cannot throw.
When FTS indexes exist, this branch is never reached — behaviour is
unchanged for users without a concurrent MCP server.
* fix(augment): guard against CONTAINS '' and add no-FTS test coverage
Blocker 1 — CONTAINS '' on whitespace-leading patterns:
pattern.split(/\s+/)[0] returns "" when the input has leading whitespace
(e.g. " ".split(/\s+/) → ["", ""]). In Kuzu, CONTAINS '' matches every
node with a name property, injecting arbitrary graph nodes into LLM context.
Fix: trim() before split, then guard on !firstWord || firstWord.length < 2.
No behaviour change for normal non-empty patterns.
Blocker 2 — zero test coverage on the FTS-unavailable code path:
The new CONTAINS fallback block (engine.ts lines 146-166) was exercised by
no existing test — all existing tests run with FTS indexes built. A second
withTestLbugDB fixture is added with no ftsIndexes, forcing searchFTSFromLbug
to return ftsAvailable: false, and asserts:
1. augment('login', ...) returns non-empty enrichment (fallback works)
2. augment(' ', ...) returns '' (CONTAINS '' guard holds)
3. augment('nxyz_notfound', ...) returns '' (no matching nodes)
4. executeQuery throwing returns '' (.catch(() => []) path)
* fix(augment): extend CONTAINS '' guard to FTS happy path and consolidate
The same split(/\s+/)[0] bug existed at line 125 (BM25 symbol filter,
FTS-available path) — a leading-whitespace pattern produced CONTAINS ''
there too, matching every node in BM25-matched files.
Fix: hoist patternFirstWord computation with trim() and the length guard
to the top of augment(), before any DB interaction. Both CONTAINS sites
(BM25 symbol filter and CONTAINS fallback) now use the single pre-validated
value. No behaviour change for normal patterns; the guard fires once for
all callers instead of being duplicated.
Also tighten the whitespace test in the no-FTS suite from 3 spaces to
4 spaces so it unambiguously exercises the patternFirstWord guard rather
than straddling the outer pattern.length < 3 boundary.
* test(augment): negative-safety test for ftsAvailable=true gate
Asserts the CONTAINS fallback does NOT fire when FTS is available but
BM25 returns zero results. Pins the safety property promised by the PR
description: behavior is unchanged for users without the read-only-DB
condition.
If anyone later loosens the gate to `symbolMatches.length === 0` alone,
this test fails.
---------
Co-authored-by: Gergő Magyar <gergomagyar@icloud.com>
* feat(cli): add --skip-skills and --index-only flags to analyze command
The `installSkills()` call in `generateAIContextFiles()` runs
unconditionally, injecting 6 skill files into `.claude/skills/gitnexus/`
even when `--skip-agents-md` is passed. This is problematic for bulk
indexing operations on read-only mirrors or third-party repos.
Add two new flags:
- `--skip-skills`: suppress standard GitNexus skill file injection
- `--index-only`: pure index mode that suppresses all file injection
(AGENTS.md, CLAUDE.md, and skills), writing only to `.gitnexus/`
This gives users three levels of control:
- `--skip-agents-md` — suppress only root context files
- `--skip-skills` — suppress only skill injection
- `--index-only` — suppress everything (pure indexing)
Discovery context: while bulk-indexing 176 repos with
`--skip-agents-md`, all 144 indexed repos were contaminated with
`.claude/skills/gitnexus/` files requiring manual cleanup.
* fix(cli): address PR #742 review — gate community skills, drop dangling refs, add tests
Bot review (#742) flagged three issues with the original commit:
1. `--index-only --skills` still wrote community-derived skill files
to `.claude/skills/generated/`. The `--skills` branch in analyze.ts
was not gated by `skipAll`, so the "skip all file injection" contract
was violated. Gate `generateSkillFiles()` with `!skipAll` so
`--index-only` truly wins over `--skills`.
2. `--skip-skills` without `--skip-agents-md` produced AGENTS.md /
CLAUDE.md that still referenced `.claude/skills/gitnexus/*/SKILL.md`
files that were never installed — every agent load incurred 6
failed reads. Pass `skipSkills` through to `generateGitNexusContent()`
and omit the standard-skill rows (and the entire `## CLI` heading
when the table is empty). Community skills, when present via
`--skills`, are unaffected.
3. No filesystem tests for `skipSkills` / `indexOnly`. Add three
regression guards to `test/unit/ai-context.test.ts`:
- `.claude/skills/gitnexus/` is NOT created when skipSkills=true
- Nothing is written when both skipAgentsMd and skipSkills are true
(the resolved-flag state from --index-only)
- AGENTS.md/CLAUDE.md routing table omits standard skill references
when skipSkills=true, but preserves the load-bearing imperative
sections (Always Do / Never Do / Resources)
* test(cli): PR 1485 review follow-ups (help text, gate test, --skip-skills docs)
- Assert --skip-skills and --index-only in analyze --help (skip-git-cli.test.ts).
- Export shouldGenerateCommunitySkillFiles; unit-test index-only+skills gate.
- Clarify --skip-skills does not suppress --skills community files; --index-only for full skip.
Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(cli): warn when --index-only silently overrides --skills
Address review findings on PR 1485 follow-ups:
- analyze.ts emits a one-line note when both --index-only and --skills
are set, so users see why a pipeline re-index ran with no skill files
written.
- index.ts --skills help text now flags the --index-only override.
- shouldGenerateCommunitySkillFiles JSDoc documents the dual role of
the gate (community skills + AGENTS.md/CLAUDE.md re-generation).
- skip-git-cli.test.ts pins the override-warning surface end-to-end.
---------
Co-authored-by: Gergő Magyar <gergomagyar@icloud.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
CI scope-parity / php parity was failing under REGISTRY_PRIMARY_PHP=0 on
three assertions added by commit af9af4a9 (U1 arity-narrowing, U3 trait
shadows parent). Per that commit's stance — "parity with the legacy DAG
is not a correctness criterion when the legacy DAG itself has the same
defect" — backporting these fixes to the legacy resolver is out of scope.
Adopt the existing sibling pattern (csharp/typescript/python use the
same helper):
- php.test.ts: switch to `const it = createResolverParityIt('php')` so
expected-failure assertions skip under legacy mode.
- helpers.ts: register three test names in
LEGACY_RESOLVER_PARITY_EXPECTED_FAILURES.php with the rationale.
Verified locally: 172 passed + 3 skipped under REGISTRY_PRIMARY_PHP=0,
175 passed under REGISTRY_PRIMARY_PHP=1, tsc clean.
* feat(embeddings): forward GITNEXUS_EMBEDDING_DIMS as dimensions in HTTP request body
When GITNEXUS_EMBEDDING_DIMS is set, include it as the `dimensions` field
in the /v1/embeddings request body. This enables Matryoshka-capable models
(OpenAI text-embedding-3-*, Cohere embed-v3, Voyage) to return truncated
vectors at the requested size.
When the env var is unset, the request body remains `{ input, model }` —
no breaking change for backends that reject unknown fields.
Adds 4 unit tests covering both paths (with/without dimensions) on both
the batch embed and single-query embed code paths.
* fix(embeddings): address review findings — strict parseInt, multi-batch test, comment wording
1. Strict parseInt validation: reject non-numeric strings like '1024abc'
by checking /^\d+$/ before parseInt (Finding 1).
2. Add multi-batch test asserting dimensions is forwarded in every fetch
call when inputs exceed batch size (Finding 2).
3. Soften JSDoc comment: backends may ignore or reject the dimensions
field rather than universally ignoring it (Finding 3).
4. Add test for invalid GITNEXUS_EMBEDDING_DIMS values.
---------
Co-authored-by: henry <zhangwei2017@unipus.cn>
Co-authored-by: Gergő Magyar <gergomagyar@icloud.com>
These were stripped by the PHP migration commit (69786b1) as
collateral damage during the rebase, but they have nothing to do
with scope resolution. They feed cluster/community detection and
Laravel route-attribute analysis at the higher analysis layers.
Restored verbatim from 69786b1~1:
- entryPointPatterns: 17 PHP idioms (Controller, handle/execute/
boot/register, REST verbs, Service/Repository, find/save/delete)
- astFrameworkPatterns: Laravel routing block with 3.0x multiplier
for Route::get / Route::post / #[Route(...)] attribute, etc.
PHP 175/175 still green; typecheck clean.
Fixes four PHP-semantic defects identified by the production-readiness
review. The bar is "graph edges must reflect what PHP actually does",
not "matches legacy DAG" — parity with the legacy DAG is not a
correctness criterion when the legacy DAG itself has the same defect.
U1: Variadic requiredParameterCount
arity-metadata.ts:49 now subtracts the variadic slot from required
count: total - optionalCount - (hasVariadic ? 1 : 0). f(int $req,
...$rest) requires 1 arg, not undefined. Adds overload-narrowing and
lookup-core arity-filter changes so resolvers actually drop candidates
that are definitively arity-incompatible (was silently rescuing
empty filter sets even when bounds were known).
U2: True transitive trait MRO
buildPhpMro now uses a BFS worklist (collectTransitiveTraits) to
flatten the trait-of-trait DAG to fixpoint instead of expanding one
level. Adds (trait_declaration body (use_declaration ...)) heritage
query so trait-uses-trait IMPLEMENTS edges are emitted at all. Fixes
3+ level trait chains silently dropping methods.
U3: parent:: bypasses composed traits
ScopeResolver gains optional buildExtendsOnlyMro hook; PHP returns
the unaugmented EXTENDS-only chain via buildPhpExtendsOnlyMro.
MethodDispatchIndex gains optional extendsOnlyMroFor accessor wired
through buildPopulatedMethodDispatch. Super-branch dispatch in
receiver-bound-calls now walks extendsOnlyMroFor when present, so
parent::method() routes to the parent class even when a composed
trait shadows the same name. Other languages leave the hook
undefined and fall back to mroFor unchanged.
U4: Namespace-aware free-call fallback
ScopeResolver gains optional isCallableVisibleFromCaller predicate;
pickUniqueGlobalCallable applies it to filter cross-namespace
candidates the caller can't reach without a use-function import.
PHP impl checks same-namespace OR explicit use-function presence,
using a side-channel namespace cache populated by
populatePhpNamespaceSiblings. Fixes the legacy DAG's namespace-
blind false-positive emissions. Existing php-calls fixture
updated: write_audit now correctly imports its targets via
use-function rather than relying on the false-positive name-only
match.
Tests:
- 175/175 PHP both flag states (REGISTRY_PRIMARY_PHP=0 and =1)
- 794/794 C#/Python/TypeScript/Go/C (no cross-language regression)
- typecheck clean
- 4 new fixtures: php-variadic-arity-minimum, php-transitive-traits,
php-parent-vs-trait, php-namespace-fallback-isolation
Plan: docs/plans/2026-05-11-001-fix-php-resolver-semantic-defects-plan.md
* fix(server): sanitize repo name to prevent argument injection
Sanitizes the extracted repository name to prevent argument injection during git clone operations and ensures compatibility with various file systems.
1. Strips leading dashes to prevent git command-line argument injection.
2. Replaces unsafe directory characters with underscores.
3. Blocks path traversal segments ('.' and '..') and Windows reserved names.
4. Fixes ReDoS vulnerability in parseRepoNameFromUrl regex.
5. Added unit tests for sanitization and path traversal edge cases.
* fix(server): expand Windows reserved name check to include extensions
- Updated sanitizeRepoName to block Windows reserved names (CON, NUL, etc.) even when they have extensions (e.g., CON.txt).
- Corrected regex and added unit tests for these edge cases to resolve CI failures on Windows.
- Ref: https://github.com/abhigyanpatwari/GitNexus/pull/1305#issuecomment-4407200914
---------
Co-authored-by: Gergő Magyar <gergomagyar@icloud.com>
Adds an optional post-resolution pass on the ScopeResolver contract:
when a member-call receiver cannot be typed by the scope chain (no
TypeRef), the language may emit CALLS edges via a workspace-wide
unique-name lookup. Runs after emitReceiverBoundCalls and before
emitFreeCallFallback, gated per-language.
PHP wires the hook to recover member calls on mixed/untyped parameters
(e.g. save(mixed $entity) calling $entity->getId()), restoring parity
with the legacy DAG. Re-enables the previously skipped save → getId
test; PHP suite is now 160/160 with no skips in both flag states.
RFC #909 Ring 3 LANG-php. Third language after Python and C# to be
migrated to the scope-based resolver pipeline.
PHP module (gitnexus/src/core/ingestion/languages/php/):
- scope-resolver.ts: ScopeResolver implementation with PHP-specific
MRO builder (traits via IMPLEMENTS edges), namespace-sibling
population, arity compatibility, and merge-binding strategy
- query.ts: tree-sitter-php .scm patterns for all PHP constructs
- captures.ts: emitPhpScopeCaptures with import decomposition,
receiver-binding synthesis, arity metadata, and PHPDoc extraction
- interpret.ts: interpretPhpImport / interpretPhpTypeBinding
- import-decomposer.ts: 1:N decomposer for use/use function/use const
including grouped use declarations
- import-target.ts: PSR-4 + composer.json resolution adapter
- namespace-siblings.ts: cross-file same-namespace visibility
- arity-metadata.ts, arity.ts: PHP arity computation and compatibility
- merge-bindings.ts: PHP binding precedence (local > import > wildcard)
- receiver-binding.ts: $this / parent type-binding synthesis
- simple-hooks.ts: bindingScopeFor hoisting for return types and
constructor-promoted properties
- cache-stats.ts: tree-sitter parse cache metrics
Registry wiring:
- pipeline/registry.ts: add phpScopeResolver alongside existing Go entry
- registry-primary-flag.ts: add PHP to MIGRATED_LANGUAGES
Shared pipeline improvements (PHP-motivated, safe for all languages):
- graph-bridge/node-lookup.ts: add Trait to isLinkableLabel so PHP
and Rust MRO builders can bridge trait defs to graph node ids
- passes/free-call-fallback.ts: add arity-narrowing to
pickUniqueGlobalCallable; when multiple global candidates exist,
narrow by parameterCount/requiredParameterCount before giving up;
fixes registry-primary languages where the semantic model is empty
Test results:
- 159/160 PHP tests pass with REGISTRY_PRIMARY_PHP=1 (scope-resolver)
- 160/160 PHP tests pass with REGISTRY_PRIMARY_PHP=0 (legacy path)
- 788/788 C#/Python/TypeScript/Go tests green
- Typecheck clean
- 1 test skipped in registry-primary mode: save->getId on mixed-typed
receiver; requires a postResolutionFallback hook in the contract
(follow-up to RFC #909 Ring 3)
* fix(windows): 32767-char tree-sitter crash + VECTOR extension SIGSEGV
tree-sitter 0.21.x on Windows crashes with SIGSEGV when parsing source
strings longer than 32 767 chars (signed 16-bit integer overflow in the
native binding). Five call sites passed raw file content without any
length guard:
- captures.ts (C# scope extraction)
- namespace-siblings.ts (extractFileStructure)
- parse-worker.ts (worker thread parse path)
- parsing-processor.ts (sequential parse fallback)
Fix: truncate at the last newline before the limit so the fragment stays
syntactically coherent. Files truncated mid-class produce ERROR roots;
captures.ts returns [] for any ERROR-root tree so the legacy DAG handles
the file silently without orphaned scope errors.
Additional C# scope fixes:
- scope-tree.ts: Module scopes may share the same range as a top-level
namespace_declaration (files with no leading `using` directives). The
rangeStrictlyContains check rejects equal ranges. Added
rangeNonStrictlyContains for Module parents.
- scope-extractor.ts: pass1BuildScopes stack-pop used strict containment;
same Module == Namespace range case caused orphaned scopes. Added
moduleAwareContains helper.
- scope-extractor-bridge.ts: empty captures from ERROR-root files still
called extractScope -> "no Module scope found" warning. Added early
return for empty/non-array captures.
- namespace-siblings.ts: three sites pushed onto binding arrays frozen by
finalize-algorithm. Fixed with spread-copy before mutation.
lbug-adapter.ts: INSTALL VECTOR in loadVectorExtension calls the KuzuDB
native extension installer, which crashes with SIGSEGV on Windows via an
unhandled error path in native code. JS try/catch cannot intercept native
signals. Skip extension loading on win32 — vector/embedding search is
unavailable on Windows but all graph index queries work correctly.
Verified on: Windows 11, Node.js 24, gitnexus 1.6.3, pcf8-game codebase
(61 757 nodes / 111 796 edges / 300 flows after fix).
* fix(windows): skip FTS extension load in pool-adapter on Windows to prevent SIGSEGV
LOAD EXTENSION fts crashes the process with SIGSEGV on Windows when the
FTS extension binary is not installed locally. This is an @ladybugdb/core
native bug — the extension loader hits an unhandled error path that raises
a native signal instead of a JS exception, so try/catch cannot protect here.
Add a process.platform === 'win32' guard in both doInitLbug and
initLbugWithDb. When skipped, bm25-index.js catches the resulting
Kuzu catalog errors (CREATE_FTS_INDEX not defined) and returns empty
BM25 results gracefully. All graph queries (cypher, context, impact)
are unaffected.
This is patch 9 of the Windows fix series for gitnexus on Windows:
patch 8 (same PR) already fixed INSTALL VECTOR SIGSEGV in lbug-adapter.ts.
pool-adapter.ts is the separate MCP-server code path that was not covered.
* fix: address codeql findings on PR #1433
The four `lastIndexOf('\n', ...)` calls were committed with a literal
newline inside the single-quoted string instead of the `\n` escape, so
the files do not parse — `tsc` and CodeQL both flagged them. Replace
the embedded newline with `'\n'`.
Also remove the two helpers that were superseded during review and
became dead code: `rangeNonStrictlyContains` in scope-tree.ts (the
equal-range carve-out is handled by `rangeStrictlyContains` +
`rangesEqual` in `canParentScope`) and `moduleAwareContains` in
scope-extractor.ts (`pass1BuildScopes` calls `canParentScope` directly).
* fix(windows): replace 32767-char truncation with chunked-input parsing
The tree-sitter 0.21.x Node binding crashes (SIGSEGV) on Windows when
parser.parse(string, ...) is handed a JS string longer than 32 767 chars.
The crash is in the bindings V8 string-to-buffer conversion and cannot
be intercepted from JS. Previous mitigation truncated source at the last
newline before that boundary, silently losing the file tail and producing
ERROR-root trees from mid-class cuts.
Switch to the callback (Parser.Input) overload via a new parseSourceSafe
helper. tree-sitter pulls source in 16 KiB chunks via repeated callback
invocations, bypassing the broken conversion path. Files are parsed in
full, no data loss, no platform-specific code path.
Removes the now-unnecessary ERROR-root short-circuit in csharp/captures.ts
and the empty-captures shim in scope-extractor-bridge.ts; both existed only
to swallow truncation-induced parse failures.
* fix(windows): cover all parse sites and correct vector-extension state
Address adversarial review on PR #1433:
1. Extend parseSourceSafe to all remaining parser.parse() call sites that
handle full file content. The first commit only converted the four
sites with active truncation hacks; cache-miss paths in
call-processor (x2), heritage-processor (x2), import-processor, and
the Go/Python/TypeScript captures + Go range-binding still called
parser.parse() directly. On Windows those would still SIGSEGV for
files > 32767 chars.
2. Stop setting vectorExtensionLoaded = true on the win32 short-circuit
in lbug-adapter.ts. The flag means "successfully loaded" and is
checked by an early-return at the top of loadVectorExtension; setting
it on the skip path made the second call return true and let
QUERY_VECTOR_INDEX run against a DB without the extension.
3. Drop the placeholder issues/... URL in the same comment.
4. Add unit tests for parseSourceSafe at boundary values: 16 KiB
(direct/callback boundary), the 32 767 Windows crash boundary,
single-line > chunk size, CRLF near boundary, and large all-Chinese
source. Confirms the callback path is correct for non-ASCII content,
which is also exercised by the existing csharp-captures large-file
test.
Researched the chunking concern: tree-sitter Node binding sets
TSInputEncodingUTF16 and divides byte_index by 2 in ByteCountToJS before
calling the JS callback, so the index argument is a UTF-16 code-unit
offset — matching String.prototype.slice. Splitting tokens across chunks
is safe by API contract; the lexer is chunk-agnostic.
* fix(windows): extend parseSourceSafe to group/embeddings + lint enforcement
Closes the remaining Windows SIGSEGV exposure flagged by the Codex
adversarial review on PR #1433. Six pre-existing parser.parse(content)
call sites bypassed parseSourceSafe and could crash the process on
Windows when a contract IDL, route file, or embedding-target source
exceeded 32 767 chars. Adds a lint rule so the regression vector closes
permanently.
Production code:
- Relocate parseSourceSafe from ingestion/utils/ to core/tree-sitter/
so group/ and embeddings/ can import without crossing into ingestion
internals. core/tree-sitter/ already houses parser-loader.ts and is
the natural shared facade. All 11 existing importers updated; no shim
left behind in the old location.
- Route through parseSourceSafe in 5 group extractors (grpc, thrift,
http-route, include, tree-sitter-scanner) and the embeddings
ensureAndParse helper.
- The seventh direct .parse() call in grpc-patterns/proto.ts:49 is a
module-load grammar smoke test parsing a 36-char literal. Trivially
safe by inspection, intentionally direct, filtered out by the lint
rule via the string-literal-arg skip.
Tests:
- 5 caller-side regression tests with a vi.spyOn assertion on
parseSourceSafe. The spy is what catches a regression: parser.parse
on a 40 000-char input succeeds on Linux/macOS, so a "no throw"
assertion alone would silently pass with the bypass reintroduced.
- The vi.mock boilerplate is centralised in
gitnexus/test/helpers/parse-source-safe-mock.ts, dynamic-imported
inside each mock factory so vitest's hoister does not race the
static import binding.
Lint:
- New custom ESLint rule gitnexus/require-safe-parse, scoped to
gitnexus/src/core/**, fails on direct <parser>.parse(<non-literal>,
...) calls and auto-fixes them to parseSourceSafe(<parser>, ...).
Skips JSON/URL/marked/Number/Math, string-literal first args
(smoke tests), test files, and the helper itself. Auto-fix rewrites
the call site only; the developer adds the import after tsc
surfaces the missing identifier — same tradeoff as
unused-imports/no-unused-imports.
Plan: docs/plans/2026-05-10-001-fix-windows-parse-safety-group-and-embeddings-plan.md
* fix(test): use mkdtempSync in http-route-extractor regression test
Address CodeQL js/insecure-temporary-file warning on the new Windows-
SIGSEGV regression test. The test was using path.join(tmpDir, "large-input")
which, when nested inside a Date.now()-based parent tmpDir, lets CodeQL flag
the directory as a predictable-name temp file with race-condition risk.
Switch to fs.mkdtempSync(path.join(tmpDir, "large-input-")) so the suffix
is a secure unique random string.
---------
Co-authored-by: Gergő Magyar <gergomagyar@icloud.com>
* feat(cursor): upgrade hooks to Cursor 2.4 postToolUse for Read/Grep/Shell coverage
Cursor 2.4 (released 2026-01-22) shipped generic preToolUse/postToolUse hooks
matching `Shell|Read|Write|Grep|Delete|Task|MCP:<tool>`, replacing the
2.3-era beforeShellExecution hook that only fired on shell commands. The
existing integration only intercepted the shell path, so Cursor users got
graph augmentation roughly 10% as often as Claude Code users — only when
the agent dropped to rg/grep instead of using its native Read/Grep tools.
This swaps the integration over to postToolUse and ports the bash+jq
hook script to cross-platform Node:
- gitnexus-cursor-integration/hooks/hooks.json: registers a single
postToolUse hook matching Shell|Read|Grep that invokes the new
gitnexus-hook.cjs.
- gitnexus-cursor-integration/hooks/gitnexus-hook.cjs: new Node hook
mirroring the safety patterns from the Claude hook (absolute-cwd
validation, .gitnexus discovery with linked-worktree fallback,
npx.cmd on Windows, end-of-options `--` marker, debug truncation,
graceful failure). Extracts the search pattern per tool kind:
Grep -> toolInput.query; Read -> file basename stripped to identifier
chars; Shell -> existing rg/grep arg parser. Emits Cursor-shape
`{ "additional_context": "..." }` on stdout — no shell, no jq.
- gitnexus-cursor-integration/hooks/augment-shell.sh: removed (Windows
incompatible, narrower coverage).
- gitnexus/test/unit/cursor-hook.test.ts: 33 regression tests covering
manifest wiring, source-level invariants (no shell:true, npx.cmd,
isAbsolute, additional_context output shape, end-of-options marker),
extractPattern coverage per tool, and behavioral early-exit paths
(empty/invalid stdin, relative cwd, no .gitnexus, unknown tool name,
short patterns, non-search shell commands, case-insensitive matching).
- README.md / gitnexus/README.md: editor-support table now lists Cursor
as Full / hooks=Yes (postToolUse), matching reality.
- gitnexus/src/cli/augment.ts and gitnexus/src/core/augmentation/engine.ts:
doc-strings updated from `Cursor beforeShellExecution` to
`Cursor postToolUse`.
Closes#1466.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(cursor): hook timeout is in seconds, not milliseconds
Cursor's `timeout` field in hooks.json is in seconds (per
https://cursor.com/docs/agent/hooks and the original integration's
`"timeout": 5`). I'd written `10000` after blindly copying the issue
body's example — that resolves to ~2.8 hours, not 10 seconds. If the
script ever hangs before reaching its inner spawnSync timeouts (e.g.
during stdin read), Cursor would have waited that long before killing
it.
Drop to `10` (seconds), matching the Claude plugin's hooks.json and
giving plenty of headroom over the inner 7s augment-CLI timeout.
Add a regression-guard assertion in cursor-hook.test.ts so a future
ms/s mixup fails fast.
Reported by Cursor Bugbot on PR #1467.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* fix(cursor): address Claude review findings — payload aliases, debug, install docs
Resolves three findings from Claude reviewer on PR #1467:
1. Cursor payload field-name uncertainty (SIGNIFICANT)
Claude flagged that the Grep `query` field is an unverified assumption
per Cursor 2.4 docs (https://cursor.com/docs/agent/hooks). Mitigated:
- Expanded Grep aliases: query | pattern | regex | q | search | searchQuery
- Added pickLongestStringValue() last-resort fallback so the hook
extracts *something* even if Cursor renames every documented field
- Added GITNEXUS_DEBUG=1 stderr logging of the raw stdin payload so
users can capture Cursor's actual contract when diagnosing silent
no-ops, and report it back if aliases drift
- Added Read alias `filePath` (camelCase variant alongside `file_path`)
- Inline comment block citing the docs URL and the uncertainty
2. Hook command path resolution + install docs (SIGNIFICANT)
Claude flagged `node ./hooks/gitnexus-hook.cjs` as relative without
documented install path. Added gitnexus-cursor-integration/README.md
with explicit install steps:
- .cursor/hooks.json + hooks/gitnexus-hook.cjs at project root
- Confirms Cursor's project-root CWD convention with doc link
- Verify steps including GITNEXUS_DEBUG capture
- Pattern-extraction contract table per tool
- Troubleshooting: not-firing, npx fallback, wrong-pattern diagnosis
3. README "Full" overclaim for Cursor (MODERATE)
Both README rows now read `Yes (postToolUse, manual install)` linking
to the new install README, accurately signaling that hooks aren't
automated by `gitnexus setup` like they are for Claude Code.
4. Shell quoted-pattern parser limitation (MINOR, documented)
Added inline comment in gitnexus-hook.cjs documenting the known
`rg "User Service"` -> `User` truncation, plus regression tests in
cursor-hook.test.ts pinning the behavior so a future change is
visible.
Test additions (33 -> 41):
- Wide-alias source coverage for Grep (query / pattern / regex / q /
search / searchQuery) plus pickLongestStringValue fallback
- Read alias coverage including camelCase filePath
- GITNEXUS_DEBUG behavioral test: stderr quiet by default, payload
echoed when env var set, stdout output contract preserved either way
- Shell quoted-pattern documented behavior tests
- Install README presence + content (.cursor/hooks.json, hooks/, debug
diagnostics)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Gergő Magyar <gergomagyar@icloud.com>
* ci(release): skip rc build on release PRs
Suppress the auto-fired Release Candidate workflow when:
1. The HEAD commit subject matches `chore: release vX.Y.Z` (the canonical
release-PR title), or
2. The squash-merged PR carries the `release` label.
Either match short-circuits the guard to should_run=false. This prevents the
rc cycle from racing publish.yml on the v-tag (as happened on v1.6.4 where
we had to manually cancel the auto-fired RC run after merging PR #1473).
Adds pull-requests: read to the guard job for the label lookup. A failed
gh API call falls through to the existing dedup logic rather than silently
suppressing rc builds.
* ci(release): address PR #1474 review — anchor regex + sanitise log echo
Two minor follow-ups from Claude's review:
1. End-anchor the release-subject regex. The previous shape
^chore: release vX.Y.Z would match noisy variants like
chore: release v1.0.0 (something unrelated). The new shape
requires either the bare title or the canonical squash-merge
(#NNNN) suffix exactly.
2. Sanitise HEAD_SUBJECT before echoing to logs. git %s strips
newlines so LF injection is impossible, but a hypothetical
subject containing ::error:: or ::set-output:: could otherwise
forge GitHub Actions annotation entries. Defence-in-depth.
Both findings flagged minor / does not block merge — applying
anyway since they are trivial.
* test(u8): de-flake regex linearity assertions
The single-trial 2x input + 3x ratio bound was razor-thin: a real macOS
CI run failed at ratio 3.01x with small=7.41ms / large=22.31ms - both
above the 5ms noise floor but close enough that single-shot scheduler
jitter pushed the ratio over.
Replace the methodology with four stacked techniques:
1. Warmup runs before timing (let the JIT tier up)
2. Median of 5 trials per measurement (eliminates GC + jitter)
3. 4x input ratio (was 2x) - linear gives ~4x, O(n^2) gives ~16x
4. 8x ratio bound with a 20ms noise floor on the LARGE measurement
Headroom: linear is expected at ~4x, bound is 8x = 2x safety margin.
A real O(n^2) regression on a 4x input would clock 16x, well outside.
Catastrophic backtracking is still caught by the absolute <500ms cap.
Verified: 10 consecutive local runs all passed.
* test(u8): address PR #1475 review — tighten floor + rename for accuracy
Two follow-ups from Claude's review:
1. Floor semantics: revert to 'skip when BOTH measurements below floor'
(AND, not single-check) and lower threshold from 20ms back to 5ms.
Median-of-5 makes 5ms reliably resolvable above performance.now()'s
~10-100us band, so the higher floor was unnecessary defense.
Closes the gap where an O(n^2) regression on a fast runner could
stay under 500ms AND below 20ms-large to escape both detectors.
2. Rename assertSubLinearRatio -> assertNearLinearScaling. The bound
is SIZE_RATIO * 2 = 8x on a 4x input = sub-quadratic with 2x
headroom over linear, not strict sub-linearity. New name reflects
the actual semantics.
* Initial plan
* chore(security): harden workflow permissions and pin Docker base image digests
Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/2ddc8f2b-7355-48cf-9a0b-c06df66c3f47
* fix(security): restore permissions: {} on publish + release-candidate workflows
These two release-publishing workflows had permissions: {} (the strictest valid form) before PR #1454, which replaced it with permissions: read-all. Every job in both files already declares its own permissions block, so the workflow-level default is only the safety net for future jobs added without one — read-all weakens that net for no benefit. Restore {} and the explanatory comment.
Scorecard's TokenPermissions check accepts both forms, so this preserves U9 compliance.
* fix(security): narrow permissions: read-all to contents: read on 13 workflows
PR #1454 added permissions: read-all to 13 workflows that previously had no top-level permissions block. read-all is Scorecard-compliant but unnecessarily broad — every job in scope only needs contents:read at the workflow level (job-level blocks already grant the writes that any job actually performs).
Snapshot of every job in the 13 workflows confirms contents:read is sufficient:
- ci.yml: quality/tests/scope-parity have explicit contents:read job blocks; save-pr-meta uses upload-artifact only (no token scopes needed); ci-status is pure shell.
- ci-e2e.yml, ci-quality.yml, ci-scope-parity.yml, ci-tests.yml: all jobs do checkout + npm + tsc/vitest/playwright/upload-artifact only; no API token scopes required.
- claude.yml, codeql.yml, dependency-review.yml, docker.yml, gitleaks.yml, pr-labeler.yml, trivy.yml, workflow-lint.yml: all jobs already declare their own job-level blocks (security-events:write, pull-requests:write, packages:write, etc.) so the workflow-level default does not gate them.
zizmor (--min-severity high) is clean on the resulting tree. Pre-existing medium findings (secrets-inherit, artipacked) are in unrelated workflows and untouched by this commit.
scorecard.yml also uses read-all but pre-existed PR #1454 and is deferred to a follow-up PR per the plan's scope boundary.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Gergő Magyar <gergomagyar@icloud.com>
* fix(security): U11 log-injection, http-to-file-access, client-side-request-forgery
U11.1: Add validateLLMBaseUrl() in llm-client.ts; called at the top of
callLLM() to reject non-http/https schemes and http:// to non-loopback
hosts before any fetch that writes LLM output to disk.
U11.2: Strip CRLF from groupDir in bridge-db.ts openBridgeDbReadOnly
before logging (defence-in-depth on top of pino's JSON escaping).
U11.3: Replace console.log with logger.debug and sanitize normalizedName
/ job.id in api.ts resolveRepo to close js/log-injection alerts.
U11.4: Add validateBackendUrl() in backend-client.ts; called inside
setBackendUrl() to reject non-http/https schemes before the URL is
stored as a fetch target, closing js/client-side-request-forgery alerts.
U11.5: Tests added:
- wiki-llm-client.test.ts: validateLLMBaseUrl happy/error paths
- server-connection.test.ts: validateBackendUrl and setBackendUrl
rejection paths
All new tests pass (30/30 wiki-llm-client, 18/18 server-connection,
30/30 bridge-db).
Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/0452a6ce-711f-4203-9ae6-5dd0b77fb157
Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>
* fix: correct IPv6 loopback check in validateLLMBaseUrl
Node's URL parser preserves brackets in hostname for IPv6 addresses
(e.g. http://[::1]:11434 yields hostname '[::1]'), so strip them
before comparing against '::1'. Add a test to cover this case.
Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/0452a6ce-711f-4203-9ae6-5dd0b77fb157
Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>
* fix: also sanitize error message in bridge-db log call
Sanitize lastErr.message (which may contain a file path from ENOENT
errors) alongside groupDir to prevent CRLF injection from error
message content. Addressed code review feedback.
Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/0452a6ce-711f-4203-9ae6-5dd0b77fb157
Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>
* fix: address security review findings — credential hygiene and test coverage
[LOW] Redact credentials from URL validation error messages:
- validateLLMBaseUrl: malformed URL no longer echoes raw input;
scheme error shows protocol only; http-non-loopback error uses
parsed.origin (scheme+host+port) instead of full URL
- validateBackendUrl: same treatment — no raw input in any error path
[INFO] Add state-preservation test for setBackendUrl:
- Proves _backendUrl is unchanged after a rejected call, covering the
validation-before-assignment ordering.
[INFO] Expand validateLLMBaseUrl adversarial test coverage:
- LOCALHOST uppercase (case-fold path)
- RFC 1918 / IMDS IPs (10.x, 169.254.x)
- Hostname-spoofing (localhost.evil.com, 127.0.0.1.evil.com, localhost.)
- Non-loopback IPv6 (fe80::1, ::ffff:127.0.0.1)
- ftp:// scheme
- Credential-hygiene assertion (sk-secret not in error message)
Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/7bb18fa2-3e66-4fe0-949f-6d493fbd351b
Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>
* style: prettier autoformat U11 security fix files
Fixes the failing 'quality / format' check on PR #1456 by running 'prettier --write' over the 6 files touched by the security fix. Formatting only — no logic change.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>
Co-authored-by: Gergo Magyar <gergomagyar@icloud.com>
* fix: pin Docker node base images and remediate bundled npm CVEs
Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/e0605c79-296e-4b3a-b6c3-4ad375950935
Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>
* fix: run trivy on docker PR changes and remove corepack
Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/4d714047-4fc1-4af1-9734-91400a15568f
Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>
* chore: add docker digest updates and normalize dockerfile comments
Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/7d980908-a823-4c28-b074-9134ec672e84
Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>
* fix: add dependabot cooldown policies
Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/a8531b8d-384b-4c54-84dd-a98b31993c44
Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>
* fix: remove unsupported dependabot cooldown keys
Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/166df50e-c2fe-4d7f-ab41-e94c703338f6
Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>
* chore(node): bump CI + engines to Node 22; centralize NPM_VERSION via build ARG
Closes the LOW findings from Claude Final Re-Review on PR #1455:
- Bump engines.node to >=22.0.0 and align all CI workflows (ci-quality,
pr-autofix, publish, release-candidate) and the composite setup
actions on Node 22. Node 20 reached EOL on 2026-04-30; the test
Docker image was already on 22.
- Centralize the bootstrapped npm version in a single ARG NPM_VERSION
per Dockerfile (cli, web, gitnexus/Dockerfile.test) so a security
bump only requires updating one default per file with a clear
cross-reference comment.
---------
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: magyargergo <11230420+magyargergo@users.noreply.github.com>
Co-authored-by: Gergo Magyar <gergomagyar@icloud.com>
* fix(autofix): verify reviewdog actually posted before claiming "click Apply"
The sticky summary comment was stating "Posted formatting suggestions
inline. Click Apply suggestion on each" even when reviewdog landed zero
inline review comments — typical case: the formatter touched lines
outside the PR's added range, so `-filter-mode=added` (correctly)
filtered everything out. The script unconditionally set `posted=true`
after running reviewdog regardless of whether any comments were
actually created, leaving the user staring at a sticky that promised
buttons that didn't exist.
The publish job now snapshots the count of `github-actions[bot]` review
comments before and after reviewdog. If the delta is zero, surface a
new `diff-no-overlap` UI state that tells the user plainly:
"Formatter found fixable issues, but they're on lines outside this
PR's added range — there's nothing to click here. Run locally:
npm run lint:fix && npm run format."
Plus a matching `gitnexus/autofix` Check Run conclusion (still neutral,
distinct title) so agents reading `gh pr checks` see the same signal.
Three states are now machine-distinguishable in the sticky's
gitnexus-autofix JSON block: suggestions-posted (delta > 0),
diff-no-overlap (delta == 0), skipped-too-large (>3k lines).
* feat(autofix): replace inline reviewdog with /autofix ChatOps button
Pivot the PR autofix UX from per-line reviewdog suggestions to a single
slash-command button. Contributors comment `/autofix` on the PR; a new
trusted workflow downloads the existing autofix patch artifact, applies
it to the PR head, and pushes a commit back.
Why:
- 3K+ diffs hit GitHub's review-comment API 406 limit -> dead end.
- Diffs where the formatter touches lines outside the PR's added range
("no-overlap") get filtered by reviewdog's -filter-mode=added -> dead
end (PR #1457 patched the lying sticky but the underlying UX gap
remained).
- Per-line click-Apply-suggestion is high-friction for big diffs and
easy to apply unevenly.
- A single `git apply` + push works at any size and lands fixes
atomically.
Changes:
- pr-autofix-publish.yml: remove `Install reviewdog` and
`Post inline suggestions` steps. Collapse three sticky states
(suggestions-posted, diff-no-overlap, skipped-too-large) into one
(fixes-available). Bump JSON schema v1 -> v2 with `apply_command`
field; all v1 fields preserved.
- pr-autofix-apply.yml (new): triggers on issue_comment with body
`/autofix`, validates body via strict regex, validates commenter
has write/admin/maintain or is the PR author, locates latest
successful pr-autofix run for PR head SHA, downloads artifact,
applies patch, pushes commit. Reacts +1/-1/eyes on triggering
comment per outcome. Idempotent (`git apply --check --reverse`
detects already-applied state).
- CONTRIBUTING.md: document v2 schema and the /autofix flow,
including the maintainer-edit requirement for fork PR pushes.
Trust posture: apply workflow runs from default-branch code only,
under issue_comment trigger. Comment body and author login flow
through env vars and pattern-matched, never interpolated into shell.
Permission gate (write/admin/maintain OR PR author) before any
artifact fetch. Fork PRs require "Allow edits by maintainers"
(GitHub-native; we don't bypass).
Net YAML: -139 lines in publish.yml, +260 in apply.yml. Removes
reviewdog binary pin and the entire review-comment API surface.
* fix(autofix): address Codex adversarial findings on PR #1458
Two findings from the Codex adversarial review of the autofix ChatOps
pivot. Both are localized YAML changes that close trust gaps the pivot
inherited from the original PR #1446 design.
U1 — Cross-verify metadata against workflow_run authority
(.github/workflows/pr-autofix-publish.yml):
Previously the trusted publisher accepted pr_number, head_sha, and
head_repo from metadata.json after only an allowlist regex. A
fork-controlled `npm run lint:fix` could have written a syntactically
valid metadata.json referencing another PR/SHA, redirecting the
write-scoped sticky/check-run onto an attacker-chosen target.
New `Verify metadata against workflow_run authority` step compares
artifact-claimed identity against:
- github.event.workflow_run.head_sha
- github.event.workflow_run.head_repository.full_name
- workflow_run.pull_requests[].number (within-repo PRs)
- gh api commits/{sha}/pulls fallback (fork PRs, where
pull_requests[] is empty)
Fail closed on mismatch — no sticky, no check-run, no override.
U2 — Lease-protected push in apply workflow
(.github/workflows/pr-autofix-apply.yml):
Previously the apply step pushed `HEAD:${HEAD_REF}` plain. A force-
push between resolve (Step 5) and push (Step 9) would silently
fast-forward an older commit graph over the contributor's newer
state.
Push now uses `--force-with-lease=refs/heads/${HEAD_REF}:${HEAD_SHA}`
against the SHA resolved earlier. Distinct `lease-failed` result code
+ retry-message reply, separated from `push-failed` (fork without
maintainer-edit) so contributors can diagnose the actual cause.
Plan: docs/plans/2026-05-09-005-fix-autofix-codex-adversarial-findings-plan.md
(local-only per repo convention).
Trust posture preserved: no new permissions, no new workflows, no
contract change. JSON v2 schema unchanged. CodeQL js/server-side-
request-forgery and template-injection posture unchanged — all new
inputs flow via env vars and pattern-matched.
* fix(autofix): close zizmor credential-persistence finding on apply checkout
actions/checkout's default behavior writes the GITHUB_TOKEN into
.git/config as an extraheader. The token then sits on disk in the
checkout directory — an actions/upload-artifact step on that
directory would leak it. We don't upload, but zizmor's
credential-persistence lint correctly flags the latent risk.
Set persist-credentials: false on the Checkout PR head step. Provide
push auth inline via `git -c http.extraheader="Authorization: Basic
<base64-of-x-access-token:TOKEN>"` so the credential never lands on
disk and never appears in process listings (the URL form
https://x-access-token:TOKEN@… is rejected here because it leaks via
ps and git remote -v).
Push lease semantics from U2 unchanged — same --force-with-lease
against the resolved HEAD_SHA, same lease-failed/push-failed/stale
result codes.
* fix(review): apply autofix feedback
ce-code-review surfaced 15 findings on PR #1458; this commit applies
the 7 with concrete fixes (#1, #2, #3, #4, #5, #9, #13). Five P2
findings (#6, #7, #8, #10, #12) are recorded as residual actionable
work for follow-up; two advisory items (#11, #14) skipped.
#1 — applied_run_id schema drift (CONTRIBUTING.md):
v2 docs claimed `state: applied` enum value and an `applied_run_id`
field that no code path emits. Trimmed docs to match what the
workflow actually writes (state: fixes-available; v1 field set as
superset). Implementing the apply-side sticky upsert that would
populate `applied_run_id` is deferred — cleaner than carrying a
contract claim with no code.
#2 — result= unset between idempotency probe and lease push
(pr-autofix-apply.yml):
After `git apply --check` passed, an early non-zero exit from
`git config` / `git apply` / `git add` / `git commit` left
`result=` unset, sending the user to the `*` "unexpected state
(`unknown`)" arm. Wrapped the apply/commit phase in a single
if-test that sets `result=apply-failed` on any failure. New
React-and-reply branch surfaces an actionable message.
#3 — permission lookup conflated transient API failures with denial
(pr-autofix-apply.yml):
`gh api … 2>/dev/null || echo "none"` swallowed 5xx, 429 secondary
rate-limit, and network failures, surfacing them as a public 👎
refusal to legitimate maintainers. Now distinguishes 404
(genuine non-collaborator) from other API failures via stderr
match. New `allowed=api-failed` state triggers a 😕 reaction with
a "transient API failure, retry" reply instead of a misleading
refusal.
#4 — lease-failure grep missed git's "remote rejected" / branch-
deleted phrasings (pr-autofix-apply.yml):
Real lease failures got classified as `push-failed` →
user told to enable maintainer-edit, which won't help. Expanded
regex to match `remote rejected` and `! [rejected]`.
#5 — broken bullet continuation in CONTRIBUTING.md release-candidate
section: rejoined the split bullet so it renders correctly.
#9 — base64 GITHUB_TOKEN bypassed GitHub's secret-masker
(pr-autofix-apply.yml):
Added `::add-mask::${auth_header}` immediately after construction
so any subsequent log line (set -x, GIT_TRACE) gets *** redacted.
#13 — misleading schema-bump comment in pr-autofix-publish.yml:
Comment claimed all v1 fields preserved exactly, but the `state`
enum was redefined v1→v2. Updated to make the migration path
explicit (v1 readers see unfamiliar schema, fall back to prose).
Residual actionable work (deferred to follow-up):
#6 locate step gh api retry; #7 artifact-expired graceful fallback;
#8 re-entrancy comment-spam guard; #10 producer-still-running UX;
#12 gh_retry wrapper for apply.yml.
Validations: yaml.safe_load OK, check-workflow-concurrency.py OK.
* fix(autofix): apply remaining ce-code-review residual findings (#6, #7, #8, #10, #12)
Pulls the deferred items from the previous review pass into this PR so
the workflow ships with full reliability + UX coverage rather than
follow-up debt.
#6 + #12 — gh_retry wrapper on idempotent GETs in apply.yml:
Permission lookup, PR metadata fetch, and workflow-run lookup are now
wrapped in the same gh_retry helper publish.yml uses (3 attempts,
linear backoff). Reaction/comment POSTs remain unwrapped (retrying
POST would dupe the resource).
#10 — producer-still-running UX:
The locate step now distinguishes three cases via `found_status`
output: success (proceed), in-progress / queued / pending / waiting
(reply ⏳ "wait for autofix run to finish"), not-found (reply 🤔
"push a commit"), api-failed (reply ⚠️ "transient API failure"). The
"no successful autofix run" message no longer fires immediately after
a fresh push while the producer is still mid-run.
#7 — artifact-expired graceful fallback:
actions/download-artifact gains `continue-on-error: true`. The apply
step distinguishes patch-file-missing (artifact expired, 1-day
retention elapsed) from patch-file-zero-bytes (formatter found
nothing). New `result=artifact-expired` case + ⏳ "push a new commit
to regenerate" reply.
#8 — re-entrancy loop guard:
After checkout but before applying, check if HEAD itself is a
github-actions[bot] `chore(autofix)` commit. If so, refuse to
re-apply (`result=loop-prevented`) with a 🔁 reply telling the user
to push a human-authored commit or revert before retrying. Prevents
formatter-config-drift loops where an automated agent watching the
sticky could pump arbitrary apply commits.
Net effect: every code path in apply.yml now sets a meaningful `result=`
that maps to a specific user-facing reaction + reply. The `*` "unexpected
state (unknown)" arm becomes truly unreachable in normal operation.
Validations: yaml.safe_load OK, check-workflow-concurrency.py OK.
* fix(autofix): refresh stale reviewdog comments + reject patches touching .github/
Two follow-up findings on PR #1458:
#1 — Stale reviewdog references in workflow header comments:
pr-autofix-publish.yml's header still described the removed inline-
suggestion path ("posts inline review-comment suggestions to the PR
using `reviewdog`", "Reviewdog reporter: github-pr-review reads
$REVIEWDOG_GITHUB_API_TOKEN…"). The Check Run permissions comment
enumerated the old outcomes (clean / suggestions-posted /
skipped-too-large) instead of the current set (clean / fixes-
available). pr-autofix.yml's header described the trusted job as
posting "inline review-comment suggestions" and the changed_lines
comment referenced the dead 3000-line cap. Refreshed all three to
describe the actual sticky + Check Run + /autofix flow.
#2 — Reject patches touching .github/ (sensitive-paths guard):
Theoretical supply-chain vector: a malicious PR could ship a custom
prettier/ESLint config that reformats workflow YAML, dependabot.yml,
or CODEOWNERS. The producer would capture those edits in
autofix.patch; a maintainer running `/autofix` would push them under
`contents: write` without human review. The default GITHUB_TOKEN
lacks the `workflows` scope so workflow-file pushes would fail at
the platform layer anyway, but as a generic `push-failed` (which
misleads users into enabling maintainer-edit). Reject early with
a specific reason.
Match runs against the patch with grep on `^(diff --git|---|+++)
[ab]?/?\.github/`. New `result=sensitive-paths` case + 🛑 reply
telling the user to apply .github/ formatter changes manually.
Documented the constraint in CONTRIBUTING.md under the /autofix
section so contributors aren't surprised when the workflow refuses
a patch that includes formatter changes to workflow files.
Validations: yaml.safe_load OK, check-workflow-concurrency.py OK.
* feat: shared resilient-fetch (retries + circuit breaker)
Add a small, runtime-agnostic resilience layer in gitnexus-shared and
migrate every backend HTTP outbound call (CLI, MCP, wiki LLM, web → backend)
through it.
Helpers (gitnexus-shared/src/integrations/):
- retry.ts — withRetry(fn, opts) with caller-supplied
retryability classification and full-jitter
exponential backoff.
- circuit-breaker.ts — closed/open/half-open per-process breaker with
injectable clock, plus a keyed registry so
callers targeting the same endpoint share state.
- resilient-fetch.ts — composed wrapper: retries 5xx + 429 + retryable
network throws, treats AbortSignal.timeout()
and 4xx (other than 429) as terminal, honors
Retry-After (capped at 30s), throws
CircuitOpenError when the breaker opens.
Migrations (no behaviour regression — all existing tests pass):
- gitnexus/src/core/embeddings/http-client.ts (covers analyze + MCP
query path) — replaces inline linear-backoff retry.
- gitnexus/src/core/wiki/llm-client.ts — preserves Azure content-filter
branch; resilientFetch handles 5xx/429.
- gitnexus-web/src/services/backend-client.ts (fetchWithTimeout helper)
— small retry budget (2 attempts, 250–1500 ms) so a dead local
backend still fails fast for the user.
- gitnexus-web/src/core/llm/settings-service.ts (OpenRouter model list).
Deliberately not migrated:
- gitnexus-web/src/services/backend-client.ts streamJob() — Server-Sent
Events stream; the existing reconnect-with-Last-Event-ID logic is
not unary-fetch shaped.
- gitnexus-web/src/components/SettingsPanel.tsx checkOllamaStatus() —
one-shot health probe; retrying delays the "Ollama not running"
error rather than improving UX.
41 new helper tests cover backoff math, breaker state transitions,
Retry-After parsing (delta-seconds + HTTP-date), 401/422 terminal
classification, and breaker fail-fast on three exhausted retry batches.
* fix(review): apply autofix feedback
Address Claude's two MEDIUM blocking findings on PR #1448 plus the
CodeQL SSRF false-positive flag.
- backend-client `fetchWithTimeout` now uses `AbortSignal.timeout()`
merged with the caller's signal via `AbortSignal.any()`. Timer-fired
aborts surface as `DOMException(name='TimeoutError')` so
resilientFetch routes them through the terminal-network branch
(no retry, no breaker hit), instead of incrementing the breaker
for user-side network slowness.
- Method-aware retry budget in `fetchWithTimeout`: idempotent verbs
(GET/HEAD/OPTIONS) keep the 2-attempt budget; POST/PATCH/PUT/DELETE
default to single-attempt so a 5xx on `startAnalyze` cannot start
a duplicate job. New `forceRetry` parameter for callers that
know-idempotent mutations (e.g. DELETE of a known-deleted resource).
- `resilient-fetch.ts` carries a documented suppression for CodeQL
js/server-side-request-forgery on the inner fetch call. Every
concrete caller passes a hardcoded URL constant or a value from
configuration (env vars, saved settings); user request input never
flows into the URL parameter.
- New test file `backend-client-retry.test.ts` covers all three
paths: GET retries on 503, POST does not retry, timeout does not
increment the breaker.
* fix(resilient-fetch): address Codex adversarial findings
Closes the three blocking issues from Codex's review on PR #1448.
U1 — Add `recordNeutral()` to CircuitBreaker.
Third outcome path that's an explicit no-op for state and the
consecutive-failure counter. Distinct from `recordSuccess` (closes
the breaker) and `recordFailure` (may open it). Used for outcomes
that are neither evidence of backend health nor evidence of
backend failure.
U2 — Route terminal-client / terminal-network through `recordNeutral`.
Previously a 401 or local timeout called `recordSuccess`, which
reset `consecutiveFailures` to 0. A 5xx → 401 → 5xx → 401 → 5xx
sequence would NEVER trip the breaker because each 4xx in between
erased the running count. Also classify external `AbortError` as
terminal-network (was retryable-network), so caller-driven
cancellation no longer retries against an already-aborted signal
or counts toward breaker failures on exhaustion.
U3 — Per-origin breaker key in web `fetchWithTimeout`.
Was hardcoded to `'web-backend'` even though `_backendUrl` is
mutable via `setBackendUrl`. Switching backend URLs after a
circuit tripped on host-A would strand the user during the full
cooldown. Key is now `web-backend:<origin>`, so each backend URL
gets its own breaker state.
Tests: +5 recordNeutral, +4 resilient-fetch (interleaved 4xx/5xx,
external AbortError, prior-state preservation), +1 web switch-backend
regression. All 70 gitnexus integration tests + 15 web tests green.
* fix(resilient-fetch): tolerate header-less fetch mocks on 429
`classifyOutcome` called `resp.headers.get('Retry-After')` directly,
which crashed when a test stubs `fetch` with a plain object like
`{ ok: false, status: 429 }` (no `headers` field). Real `Response`
always has Headers, so this surfaces only in test setups, but the
helper has no business assuming caller-side correctness on this — the
defensive guard is cheap and a missing `Retry-After` falls through to
exponential-backoff retry like any 429 without the header.
Surfaced by `gitnexus/test/unit/http-embedder.test.ts > retries on
rate limit`, which the embeddings migration exercises against a
plain-object 429 stub. Locked in with a new
`classifies 429 from a header-less fetch mock without throwing` case.
* fix(review): apply autofix feedback
Closes findings from the third multi-agent review pass on PR #1448.
#1 (P1) callLLM had no per-attempt timeout
Wiki LLM calls passed no `signal` to resilientFetch; each of three
retry attempts could hang indefinitely on a frozen TCP connection.
Add `signal: AbortSignal.timeout(60_000)` so the per-attempt budget
matches what http-client.ts and backend-client.ts already provide.
#2 (P2) drop dead `lastRetryableResp` post-loop fallback
Variable was set in one switch arm but only read in unreachable code
after the loop. The retry loop always returns/throws on every
iteration. Keep only the defensive `throw` so TypeScript's
control-flow analysis still sees `Promise<Response>` as the return.
#5 (P2) gate test-only exports behind a subpath
`__resetBreakerRegistry__` and `classifyOutcome` were reachable from
the main `gitnexus-shared` barrel — production code calling
`__resetBreakerRegistry__` from a tool implementation would silently
nuke every circuit breaker process-wide. Move to a new
`gitnexus-shared/test-helpers` subpath export. Production callers
see the cleaner public API; tests import via the explicit
`gitnexus-shared/test-helpers` path.
#6 (P2) exhaustiveness guard on Outcome switch
Add a `default: const _: never = outcome` arm so a future sixth
`Outcome.kind` won't compile silently — it'll surface at the switch
site rather than fall through to a retry/no-retry default.
#9 (P3) document cumulative wall-clock budget
Add a "Cumulative wall-clock budget" paragraph to resilientFetch's
JSDoc explaining the worst-case total wait (`maxAttempts × (per-attempt
timeout + capDelayMs)` ≈ 60s with defaults) and pointing callers at
outer `AbortSignal.timeout()` when they want a tighter bound.
Deferred to follow-up PRs (per review's Auto-resolve recommendation):
- #3 idempotency knob to shared API (forceRetry into ResilientFetchOptions)
- #4 publish.ts migration to resilientFetch
- #7 parseRetryAfter past-HTTP-date / negative-seconds asymmetry
- #8 recordNeutral counter time-decay (documented breaker semantic)
* fix(circuit-breaker): gate half-open to a single in-flight probe
Closes the Codex adversarial-review finding on PR #1448 that flagged a
recovery-time thundering herd: when cooldown expired, every concurrent
caller transitioned the breaker to half-open and probed the still-
recovering dependency in lockstep, defeating the breaker's "fail fast"
promise.
U1 — probe-permit gate in CircuitBreaker.check()
Added a `probeInFlight: boolean` field. After cooldown expires, the
first `check()` admits the probe and consumes the permit; subsequent
callers throw `CircuitOpenError` with a configurable
`halfOpenRetryAfterMs` (default 1000ms) until the probe resolves.
Critical design point: `recordNeutral` now RELEASES the permit but
does NOT transition state. Without that split, a single `TimeoutError`
from per-attempt `AbortSignal.timeout` (which routes through neutral
classification) would permanently park the breaker in half-open. By
separating permit-release from state-resolution, we keep the
"neutral doesn't claim health" semantic without creating that wedge.
Other changes:
- `halfOpenRetryAfterMs` is now a constructor option for consumers
with long-running protected ops (LLM streaming, large uploads).
- `getState()` is documented as a pure read; the implicit
Open -> Half-Open transition lives in `check()` only, so tests
that inspect state never inadvertently consume a probe permit.
- `isProbeInFlight()` test-only accessor for assertion clarity.
- JSDoc on `check()` records the JS event-loop atomicity dependency
and the load-bearing `try/finally` pairing invariant.
U2 — End-to-end concurrency regression through resilientFetch
Three new scenarios in resilient-fetch.test.ts (26 -> 29):
- 3 concurrent calls + probe gets 200 -> 1 hits fetch, 2 throw
CircuitOpenError, breaker closes.
- 3 concurrent calls + probe gets 503 -> ResilientFetchExhaustedError
on probe; concurrent callers see halfOpenRetryAfterMs (1000ms);
fresh caller after probe resolves sees the FULL new cooldown
(10000ms), not the probe-in-flight default.
- Probe cancelled mid-flight via AbortError -> permit released,
state stays half-open, next caller becomes the new probe and
succeeds.
Plus 9 new circuit-breaker unit tests (16 -> 25) covering the permit
gate, recordNeutral-releases-permit semantic, fresh-cooldown distinction,
default vs configurable halfOpenRetryAfterMs, getState() purity, and
the three-probes-via-neutrals chain.
Total integration test count: 70 -> 82. All 106 gitnexus + 15 web
tests pass; both packages typecheck.
Maintainer decisions (deferred per plan 003 Open Questions):
- Plan 002's deferral judgement was reversed on Codex's argument
without new measurement / incident data. The reversal is defensible
on principle (Hystrix / Resilience4j alignment) but lacks workload-
driven evidence.
- Probe-blocked callers throw silently (no log / event hook). R4's
"no new public API" prevents adding observability; loosen if a
debug log on probe-blocked is wanted.
* refactor(embeddings): replace bespoke HF breaker with shared CircuitBreaker
Deleted the local `HfDownloadCircuitBreaker` class and the manual
retry loop in `withHfDownloadRetry`. Both are now backed by the
shared `gitnexus-shared` primitives:
- `hfDownloadCircuit` is `new CircuitBreaker({ failureThreshold,
cooldownMs, key: 'hf-download' })` — same state machine as before
PLUS the single-permit half-open gate that prevents recovery-time
stampedes when CLI + MCP embedders concurrently re-load the model.
- `withHfDownloadRetry` delegates the loop to `withRetry` from the
shared package. Per-attempt timeout (`withDownloadTimeout`),
network-vs-non-network classification, circuit recording, and the
`onRetry` callback wire through `withRetry`'s `isRetryable`
callback.
Behaviour preserved:
- Pre-flight `CIRCUIT_OPEN_TAG` rejection when the breaker is open.
- Mid-loop `CIRCUIT_OPEN_TAG` "opened after N consecutive failures"
when a network error trips the threshold.
- Non-network errors (e.g. CUDA unavailable) bypass retry and go
through `recordNeutral` instead of resetting the breaker's
failure-count progress.
- `onRetry(attempt+1, max, err)` fires only when there's a next
attempt, matching the prior semantic.
Generic CircuitBreaker gained two inspection accessors:
- `getOpenedAt(): number | null`
- `getCooldownMs(): number`
Used by `withHfDownloadRetry` to compute `secsUntilReset` without
consuming a probe permit (which `check()` would do).
Test consolidation: the 7 bespoke `HfDownloadCircuitBreaker`
state-machine tests in hf-env.test.ts were 1:1 duplicates of
existing tests in `circuit-breaker.test.ts` and were deleted.
Remaining 42 hf-env tests all pass; full integration sweep (148
gitnexus + 15 web) green.
