mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-09-09 22:33:39 +00:00
* fix(ci): stop the review agent rejecting its own graph-backed reviews
The context-evidence gate only counted a `context` call when the call
itself passed `file_path` equal to a changed path. The review skill
teaches plain `context({name})`, so 17 of the 26 review-agent run
failures were complete, graph-backed reviews thrown away after full
model spend, with no log line saying which invariant failed.
Prove the evidence from the result instead: `status=found` plus a
`symbol.filePath` inside the repo-scoped changed-path set. Every other
check stays exactly as it was - strict JSON, orchestrator-only turns,
result ordering, duplicate tool-id rejection - and the `repo` argument
still selects the head or the merge-base path set.
Same failure inventory, smaller classes:
- rejection now logs why (in-scope, out-of-scope, sidechain, unresolved
and off-path counts plus up to three sanitized paths), and the
envelope error names the message count and first-message shape
- Glob/Grep leave the tool set: they were enabled through `--tools` but
never allow-listed, so every lane call was denied and burned turns
- both pinned `npm ci` installs retry three times; one registry
ECONNRESET killed a whole run
- the prompt matches the new contract and asks for the structured body
even when the analysis is incomplete
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(skills): mirror the review-skill tool-set change into the shipped copies
The npm package, Claude plugin, and Cursor integration ship byte-identical
copies of .claude/skills/gitnexus-review, and the drift guard compares them.
Dropping Glob/Grep from the lane frontmatter and the SKILL.md sentence only
landed in the canonical tree.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(ci): stop one junk context result discarding a proven review
Tri-review of this PR found that the previous commit fixed one spurious
rejection and created another. Widening evidence candidacy from "the call
that named a changed path" to "every orchestrator context call" also
widened the *strict-parse* surface: `contextResultProvesChangedPath`
throws rather than returning false, so a single malformed payload
anywhere in the transcript now discarded a review that an earlier call
had already proven. The MCP makes that reachable without any misbehaving
model - `GITNEXUS_MCP_DEFAULT_MAX_TOKENS=12000` truncates any context
payload over ~48 KB mid-JSON and appends a marker - and it also destroyed
docs-only runs that the `no_indexable_changed_symbols` mode exempts.
Reproduced by running the workflow's own embedded script on both trees:
a proving evidence call followed by one truncated exploratory call gave
`failure_code: null` on the base and `invalid_execution_transcript` on
the head; it is `null` again here.
- payload-shape failures are caught and counted (`malformedResults`)
instead of thrown; transcript-structural invariants (envelope, tool
shapes, duplicate ids, empty tool_result) still fail closed
- diagnostics gained the reasons they were blind to: errored results,
results that arrived out of order or via a sidechain, unanswered
in-scope calls, and malformed payloads. A rejection can no longer
print an in-scope call with every reason at zero
- a deletion-only PR no longer registers head-scoped candidates that can
never be satisfied: an empty eligible set is out of scope, not a result
"outside the changed paths"
- the mandatory-body prompt clause now pairs with a required `complete`
boolean. An incomplete analysis publishes its partial body labelled
`incomplete_analysis` instead of passing as an accepted review
- `Agent(a,b,c)` is split into six separate `Agent(x)` rules: the pinned
base action parses allowedTools with `.flatMap((v) => v.split(","))`
(parse-sdk-options.ts at 3553f843), which shattered the grouped rule
into `Agent(ci-correctness-lens`, four bare names, and
`ci-critic-lens)` before the SDK saw it. Pre-existing and unproven at
runtime, but the split form is correct under either reading and lets
the header's dispatch canary actually prove something
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(ci): require a line range for context evidence
The tri-review's adversarial lane executed `context({name: 'AGENTS.md'})`
and had the result accepted: the gate checked only that the resolved
filePath was in the changed set, so a bare File node passed for a review
of that file's contents. The trusted prescan already defines an indexable
symbol as one with startLine and endLine, so require the same here.
Pre-existing rather than introduced by this branch, but it is the same
"what counts as proof" surface the rest of this PR tightens.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(ci): close the remaining tri-review findings
Addresses every finding the tri-review left open after 0432214d and
1d9f2d75, across both engines.
Reliability and maintainability (Codex ce, ce-reliability, ce-maintainability):
- both pinned `npm ci` installs now call one shared
`.github/scripts/npm-ci-retry.sh` instead of two near-identical 12-line
blocks that differed only in a label
- each attempt runs under `timeout` (default 600s, overridable), so a slow
registry can no longer crowd the model review out of the job's budget
- the helper distinguishes a timeout kill (124) from an npm rejection in
its log
Test coverage (ce-testing, Codex swarm P3, ce-security, swarm test-ci):
- the retry helper is now exercised behaviourally with a stub npm: first-try
success runs once, two failures recover on the third, three failures exit 1
- a non-string `symbol.filePath` is a clean reject, not a type error
- an adversarial resolved path (ESC, newline, `::set-output`, RTL override)
is proven sanitized before it reaches the job log
- the envelope error's shape string is asserted
- an in-scope call whose result never arrives is counted, not silent
- install flags that keep the runtime inert (`--ignore-scripts`, `--prefix`,
the lock-bound registry) are asserted against the helper they moved into
Correctness and clarity (risk-architect, ce-standards):
- the prompt now tells the model to prefer the uid form or pass file_path
when a bare name could resolve into an unchanged file, which was the
narrower off-path failure mode the gate rewrite left behind
- `contextResultProvesChangedPath` -> `contextResultProvesEligiblePath`,
matching the set-membership contract its sibling was renamed for
- the transcript fixture's default no longer carries a `file_path` the gate
ignores, which implied the opposite of the contract
- SKILL.md says "file reads" rather than naming a CLI-specific tool, per
the CLI-neutrality rule in AGENTS.md; mirrored to all three shipped copies
- the interactive-swarm README notes the CI lanes are narrower
Publisher (ce-reliability residual, pre-existing):
- the publish job no longer gates the whole job on authorization, so a
request rejected at normalization no longer strands the "review in
progress" marker on the PR forever. Publication stays authorization-gated
at the step; only the marker cleanup is unconditional.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(ci): ship the install helper executable
The extracted helper was committed 100644, so the workflow's direct
invocation would have failed on the runner with permission denied - a
break introduced by the extraction itself, invisible to every existing
assertion. Set the mode and pin it with a test.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Gergo Magyar <gergomagyar0@gmail.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
40 lines
1.6 KiB
Bash
Executable file
40 lines
1.6 KiB
Bash
Executable file
#!/usr/bin/env bash
|
|
# Install a lock-pinned runtime, retrying only what a transient registry fault
|
|
# can change. `npm ci` re-creates node_modules from the committed lockfile and
|
|
# re-verifies every SHA-512 integrity on each attempt, so a retry can only
|
|
# reproduce the identical tree — never a different one. Each attempt is bounded
|
|
# so a hung registry cannot eat the job budget the model review needs.
|
|
#
|
|
# Usage: npm-ci-retry.sh <label> <runtime_dir> <npmrc>
|
|
set -euo pipefail
|
|
|
|
label="${1:?usage: npm-ci-retry.sh <label> <runtime_dir> <npmrc>}"
|
|
runtime_dir="${2:?missing runtime dir}"
|
|
npmrc="${3:?missing npmrc}"
|
|
attempts="${NPM_CI_RETRY_ATTEMPTS:-3}"
|
|
attempt_timeout="${NPM_CI_ATTEMPT_TIMEOUT_SECONDS:-600}"
|
|
|
|
for attempt in $(seq 1 "${attempts}"); do
|
|
if timeout "${attempt_timeout}" npm ci \
|
|
--prefix "${runtime_dir}" \
|
|
--userconfig "${npmrc}" \
|
|
--ignore-scripts=true \
|
|
--audit=false \
|
|
--fund=false \
|
|
--registry=https://registry.npmjs.org/; then
|
|
exit 0
|
|
fi
|
|
status=$?
|
|
if [[ "${attempt}" -ge "${attempts}" ]]; then
|
|
echo "The pinned ${label} install failed after ${attempts} attempts (last exit ${status})." >&2
|
|
exit 1
|
|
fi
|
|
# 124 is `timeout`'s own signal that the attempt was killed, not that npm
|
|
# rejected the lock; both are retried, but the log says which happened.
|
|
if [[ "${status}" -eq 124 ]]; then
|
|
echo "The pinned ${label} install exceeded ${attempt_timeout}s; retrying (${attempt}/${attempts})." >&2
|
|
else
|
|
echo "The pinned ${label} install failed (exit ${status}); retrying (${attempt}/${attempts})." >&2
|
|
fi
|
|
sleep "$((attempt * 5))"
|
|
done
|