Post-v0.3.0 batch: accepted skill fixes, P0 fleet, harness hardening - #29
Merged
Merged
Conversation
…the three consensus rows The W1-2b registration goes into EVALS before any SKILL.md changes: target row set (9 rows, relabeled rows excluded), fresh blind two-rater instrument (orchestrator-run), accept band [8,9] consensus-correct vs the 7/9 sealed baseline, bm25 floors as guardrails, out-of-band rule = revert descriptions, keep relabels. The three two-rater-consensus label contradictions (M040 M043 M062) are relabeled with a RELABEL log in the corpus header; prompts/provenance/split untouched, no rows added. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ADD-only, 1-2 sentences each, per the W1-2b registration in EVALS: mcp claims the tool-health / index-staleness / "is my setup broken" moment; orient claims --doctor and --skipped by flag name; navigate claims the N-way --connect relate moment; before-you-build claims the --lego interface-implementors moment (the moment is multi-symbol NEW work against a boundary, and the labeled corpus row for it is a before-you-build row); fresh-eyes and quality-bar each gain one crisp boundary sentence hung on the moment (diagnose the shape of code you didn't write vs name the fix for a measured shape / judge what you just wrote). No prose removed, no clause replaced: every existing clause is either gate-pinned (agentloopcodexcheck markers, skilltruthcheck counts) or previously measured discriminative text, so the length-penalty tradeoff was taken as pure ADD and is reported, not judged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ubliccheck arm 3 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…led regex scan as over-refused pargates.py's flat 300s per-gate cap misjudged two HONEST long runs under ASan on a cold cache (cppbenchcheck ~856s, regexbombcheck ~804s) as timeouts. Replace the flat cap (and the prior single-purpose SLOW_TIMEOUT_SEC/slow-set pair) with a declared GATE_BUDGET_SEC table so each override documents itself: the six git-HEAD-build gates keep 900s, cppbenchcheck/regexbombcheck get an honest 1200s, everything else keeps the 300s default. The TimeoutExpired message now names its own declared budget so a red is self-explaining. Separately, regexbombcheck.sh's safeCase() (and the no-bait structural check) fell straight through capRun's "TIMEOUT" sentinel into the generic failure branch, printing a killed/timed-out scan as "OVER-REFUSED: ... exit TIMEOUT" — a wrong verdict, not just a timeout. Give both call sites the same TIMEOUT-vs-verdict branch bombCase() already has, so a kill reports as TIMEOUT/KILLED and never as a refusal-precision finding. test/pargatescheck.sh is new: static assertions on the budget table plus a functional run of the real pargates.py logic (via two throwaway patched copies, numbers-only, against a synthetic slow gate) proving a gate times out at its declared budget and that a GATE_BUDGET_SEC override actually changes the outcome. Registered in test/regression.sh's loop (384 -> 385); the three gate-count quotes in docs/EVALS.md recomputed from the actual loop length and verified via test/manifestcheck.sh.
…ver-touch invariant
evictOldCacheFamily's matches() lambda (src/quality.h) accepts EITHER ".bin" or ".cache" as a
family-member suffix, but every existing evictioncheck.sh arm only ever seeded ".bin" — the
".cache" half of that OR had no gate coverage at all. Separately, advisory edit locks
(src/mcpedit.h editLockPath) deliberately live under cacheDirLadder()/locks/, protected from the
sweep by two independent layers (the shard walk only recurses into exactly-2-hex-char subdirs,
and ".lock" fails matches()'s suffix check even if it did) — again with no gate asserting either
protection held.
Adds a Y5 section to evictioncheck.sh, isolated in its own TMPDIR: (a) an old .cache file IS
evicted, (b) a fresh .cache file is NOT, (c) an ancient file under locks/ is NEVER removed even
though its "ripwire-" prefix matches the family. (d) .bin behavior is unchanged, already covered
by the pre-existing arms this section leaves untouched.
Arms (a)/(b)/(c) all PASS pre-change — this closes an assertion gap, not a bug. Verified with
mutation-style evidence instead: (1) with matches() temporarily narrowed to ".bin" only, arm (a)
turns red ("old .cache blob survived"); (2) with the shard-walk widened to recurse into ANY
subdirectory (not just hex-named ones) and matches() loosened to prefix-only, arm (c) turns red
("ancient file under locks/ was REMOVED"), while every other arm — including (a)/(b) — still
passes. Both mutations were local-only, rebuilt, observed red, then `git checkout -- src/quality.h`
to restore; `git status`/`git diff` on src/quality.h confirmed clean before this commit.
…ee it --rank-by=churn counts every commit in its window equally, so a file rewritten fifteen times two years ago outranks one rewritten twice last week. Widening or narrowing the window only moves the cliff; the quantity decays smoothly and the window is a step function over it. --rank-by=churn-decay weights each commit 0.5^(age_days/90) instead. The half-life is a conventional choice, not a measurement on the corpus, so it is disclosed in window= and in the legend rather than buried in a constant. The load-bearing part is the anchor. "age" comes from HEAD's own committer epoch (gitHeadCommitEpoch, extracted from the short-horizon miner that was already the only reader of it), never std::time() — otherwise this would be the one verb whose output changed overnight on an unchanged tree. Because the decay makes a cut-off unnecessary, the default walk carries no --since at all and is a pure function of (repo, HEAD); an explicit --since still narrows it, and that arm is wall-clock-relative by the user's own choice, exactly as churn's already is. Gate first: test/churndecaycheck.sh, 10 red arms against the pre-change binary. Its arm 5 is the one that matters — two repos with identical relative commit spacing and absolute dates five years apart must rank identically, which a wall-clock anchor cannot do. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ed per its own registration Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…dy prints
Every default map tags file nodes with an architecture layer (layer="render",
arch.h's built-in directory-name taxonomy), but the composable query language
could not read it — so "which render-layer functions have 10+ callers?" was
unaskable even though the map prints the answer's first half on every run.
layer(SET, NAME) closes it, over the SAME builtinLayer() the serializer calls.
One taxonomy, two surfaces: a second definition here is exactly how the query
and the map come to disagree about what a layer is on one tree.
Two refusal arms, because the causes are different facts:
- a word outside the closed vocabulary is a typo. Refuse, name the
vocabulary. §P0.5b's rule for name(), applied to the grammar's other
literal.
- a valid word against a tree where NO path names a layer means the question
cannot be asked here. count="0" would read as "there is no render code" and
an agent that believes it goes looking somewhere else.
A valid word against a tree that IS layered but has no members in that layer
stays a measurement and reports count="0" — the line query.h already draws.
layer() does not read a --arch=FILE rules file. --arch is a verb and outranks
--graph-query in the dispatch chain, so the two never run in one invocation; a
taxonomy the user could pass but the query could never see would be a worse lie
than a documented limit, so --help states it.
Gate first: test/layerquerycheck.sh, 10 red arms against the pre-change binary.
Its fixtures are built in a temp dir, not committed under test/ — `test/` is
itself a layer dir, so a committed fixture would tag every file layer="test"
and the discrimination arms would pass for the wrong reason.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e corpus findClonesType3 returns PAIRS by construction, so three functions that are all near-copies of each other arrive as three rows of two. A reader counting rows concludes there are three duplication problems where there is one cluster of three — the detector is right and the report is misleading. Union-find over the pair graph recovers the cluster: gid= on every row, clone_groups= on the root, computed over the FULL detector output rather than the displayed window (a summary that shrank with --limit would be a paging artefact, not a measurement). With components in hand the corpus can be priced. dup_pct = duplicated LOC / total LOC, and both operands ship as attributes so the number can be checked rather than believed. The per-cluster rule is the disclosable choice: every member's loc EXCEPT the largest member's — one instance is the code you keep, the rest is the redundancy, so a 3-clone cluster counts its lines twice. Not three times (a corpus that is one function copied twice would read 100% duplicated, a number nobody can reduce) and not once (a 3-way clone would price the same as a 2-way). The largest member is the representative, which minimises the reported redundancy. All three derived counts are FLOORS — the Type-3 pair list is capped upstream, and a dropped pair is a cluster left unmerged and a percentage that is too low. counts_floor="1" says so, and the legend carries the whole definition. Gate: test/type3clonecheck.sh PART 2, 8 red arms against the pre-change binary, with the arithmetic pinned on a hand-computed corpus (two identical equal-LOC bodies, dup_pct 50.0) rather than on whatever the code happened to produce. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ccumulator PART 2 records every drifted attribute in p2fail and reports them all in one run, so the gate is no longer fail-fast and arm (B) can reach its terminal region and check it forced and clean. That is strictly stronger than the pin, so the row is retired rather than reworded — the deckclaimcheck precedent. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
churnRankedGraph and runDefaultMap moved in src/main.cpp when churn-decay landed. readmeexamplecheck compares the README block against live output, so the recorded rows are data, not prose. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
# Conflicts: # docs/EVALS.md # test/regression.sh
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe pull request adds time-decayed churn ranking, architecture-layer graph queries, clone component metrics, expanded validation gates, per-gate timeout budgets, and documentation updates. ChangesAnalytics features
Validation harness
Documentation and skill routing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant gitmine
participant MapSerialization
CLI->>gitmine: select churn-decay and resolve history scope
gitmine->>gitmine: mine HEAD-anchored decayed file weights
gitmine->>CLI: return smoothed symbol prior
CLI->>MapSerialization: provide churn-decay rank label
MapSerialization->>CLI: emit rank metadata and legend
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
test/pargatescheck.sh (1)
104-116: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThe 6 s override against a 4 s sleep is a narrow margin and can flake.
Copy B gives
probequickgate.sha 6 s budget for a 4 s sleep. The margin is 2 s.test/regression.shandtest/pargates.py -j 6run this gate alongside ASan gates that saturate CPU. Python startup,subprocesssetup, and scheduler delay can consume that margin on a loaded 4-vCPU runner. The gate then reports "override not honored" when the override was honored.Widen the ratio instead of the absolute values. The arm proves only that
6 > 4 > 2.♻️ Proposed change
-patchPargates "$COPYB" '"probequickgate.sh": 6,' +patchPargates "$COPYB" '"probequickgate.sh": 60,'Keep
DEFAULT_TIMEOUT_SEC = 2so arm A still kills the same 4 s sleep. Arm B then still proves the lookup changed the outcome, with a margin large enough to survive a loaded runner.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/pargatescheck.sh` around lines 104 - 116, Increase the Copy B override for probequickgate.sh in the pargatescheck test while keeping DEFAULT_TIMEOUT_SEC at 2 seconds and the gate’s 4-second sleep unchanged. Preserve the test’s proof that the override exceeds the runtime and allows pargates.py to exit successfully, but use a substantially wider margin than the current 6-second budget.test/pargates.py (1)
122-135: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winA timed-out gate can leave orphan processes behind.
subprocess.run(..., timeout=limit)kills only the direct child, which isbash. Each gate script spawnsripwire(often under ASan, sometimes in the background, ascapRun()intest/regexbombcheck.shdoes). Those grandchildren survive the kill and keep consuming CPU and memory for the remainder of the parallel run. With-j 6and a 1200 s budget, one hung gate can slow every other gate and produce further false timeouts.Start the gate in its own process group and kill the whole group on timeout.
♻️ Proposed fix
def run(g): env = dict(os.environ, RIPWIRE_BIN=binp) limit = GATE_BUDGET_SEC.get(g, DEFAULT_TIMEOUT_SEC) t0 = time.time() try: - p = subprocess.run( - ["bash", os.path.join(testdir, g)], - cwd=root, env=env, capture_output=True, timeout=limit, - ) - rc, out = p.returncode, (p.stdout + p.stderr).decode("utf-8", "replace") - except subprocess.TimeoutExpired: - # the budget itself is part of the message -- a red names its own declared budget instead of - # making the reader go look it up in GATE_BUDGET_SEC. - rc, out = 124, f"TIMEOUT after {limit}s (declared budget={limit}s)" + p = subprocess.Popen( + ["bash", os.path.join(testdir, g)], + cwd=root, env=env, + stdout=subprocess.PIPE, stderr=subprocess.PIPE, + start_new_session=True, + ) + try: + so, se = p.communicate(timeout=limit) + rc, out = p.returncode, (so + se).decode("utf-8", "replace") + except subprocess.TimeoutExpired: + # kill the whole group so the gate's own children (ripwire, background scans) die too + try: + os.killpg(p.pid, signal.SIGKILL) + except ProcessLookupError: + pass + p.communicate() + # the budget itself is part of the message -- a red names its own declared budget instead + # of making the reader go look it up in GATE_BUDGET_SEC. + rc, out = 124, f"TIMEOUT after {limit}s (declared budget={limit}s)"This needs
import signalat the top of the file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/pargates.py` around lines 122 - 135, Update run() to launch each gate in its own process group, adding the required signal import, and replace subprocess.run’s timeout-only handling with process-group termination on subprocess.TimeoutExpired. Kill the entire group rooted at the gate process before recording the existing timeout result, while preserving normal return-code and output handling.test/churndecaycheck.sh (1)
53-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHarden the fixture against local git configuration, and guard the shifted repo too.
Two points:
mkrepoinherits the user's global git config. Ifcommit.gpgsign=true,user.signingkey, or a global hook template is set,git commitfails. All output is redirected to/dev/null, so the repo ends up empty. Pass the overrides on the command line to make the fixture hermetic.- The presence guard at Line 93 covers only
$WORK/recent. If$WORK/shiftedfails to build, arm 5 reports "the decay is reading the system clock" while the real cause is a broken fixture. Guard both repos.♻️ Proposed hardening
- git -C "$dir" init -q 2>/dev/null + git -C "$dir" init -q 2>/dev/null git -C "$dir" config user.email rw@example.invalid git -C "$dir" config user.name ripwire-gate + git -C "$dir" config commit.gpgsign false + git -C "$dir" config core.hooksPath /dev/null-[ "$( git -C "$WORK/recent" rev-list --count HEAD 2>/dev/null )" = 8 ] \ - && ok "fixture guard: recent repo has 8 commits" \ - || no "fixture guard: recent repo does not have 8 commits (got $( git -C "$WORK/recent" rev-list --count HEAD 2>/dev/null ))" +for r in recent shifted; do + n="$( git -C "$WORK/$r" rev-list --count HEAD 2>/dev/null )" + if [ "$n" = 8 ]; then + ok "fixture guard: $r repo has 8 commits" + else + no "fixture guard: $r repo does not have 8 commits (got '$n')" + fi +done🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/churndecaycheck.sh` around lines 53 - 95, Make mkrepo hermetic by passing per-invocation git config overrides to every git commit so global signing settings, signing keys, and hook paths cannot prevent fixture creation; keep the existing local identity configuration. Extend the presence guard to validate that $WORK/shifted also contains all 8 commits before running assertions, reporting its actual count on failure.test/regression.sh (1)
198-198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd reverse gate-list validation to
test/manifestcheck.sh. The loop skips missing scripts, and the manifest check does not detect duplicate names.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/regression.sh` at line 198, Add reverse gate-list validation in test/manifestcheck.sh: compare the manifest’s gate entries against the gate names in the regression.sh loop, fail for any manifest gate missing from that loop, and reject duplicate gate names in the manifest. Preserve existing validation while ensuring both omissions and duplicates cause a failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/COMMANDS.md`:
- Line 1528: Update the “Shaped by” entry for --arch in COMMANDS.md to remove
--graph-query, since runArchViews executes before runGraphQuery and they do not
compose; also verify whether --deps actually shapes --arch and remove it if it
does not.
In `@src/clones.h`:
- Around line 933-946: The groupClones function currently includes short
function and method bodies in totalLoc even though findClones and
findClonesType3 exclude them. Pass the candidate universe or minTokens into
groupClones and apply the same minimum-token filter used by those detectors when
accumulating totalLoc, while preserving the existing symbol-kind and
body-validity checks.
In `@src/gitmine.h`:
- Around line 1553-1601: Update the churn history evidence tracking so
outAnyHistory reflects only commits that contribute weights: move the
anyCommit=true assignment from the __C__ marker handling into flush(), setting
it only when the deduplicated cur collection passes the existing 1..maxFiles
gate. Keep marker parsing and weight accumulation unchanged.
In `@test/type3clonecheck.sh`:
- Around line 212-217: Update the xmllint-unavailable branch in the extended
clones XML validation check to mark the verification as skipped rather than
calling p2no, so the final gate does not fail when the optional dependency is
absent; keep the existing failure behavior when xmllint is present but rejects
the output.
---
Nitpick comments:
In `@test/churndecaycheck.sh`:
- Around line 53-95: Make mkrepo hermetic by passing per-invocation git config
overrides to every git commit so global signing settings, signing keys, and hook
paths cannot prevent fixture creation; keep the existing local identity
configuration. Extend the presence guard to validate that $WORK/shifted also
contains all 8 commits before running assertions, reporting its actual count on
failure.
In `@test/pargates.py`:
- Around line 122-135: Update run() to launch each gate in its own process
group, adding the required signal import, and replace subprocess.run’s
timeout-only handling with process-group termination on
subprocess.TimeoutExpired. Kill the entire group rooted at the gate process
before recording the existing timeout result, while preserving normal
return-code and output handling.
In `@test/pargatescheck.sh`:
- Around line 104-116: Increase the Copy B override for probequickgate.sh in the
pargatescheck test while keeping DEFAULT_TIMEOUT_SEC at 2 seconds and the gate’s
4-second sleep unchanged. Preserve the test’s proof that the override exceeds
the runtime and allows pargates.py to exit successfully, but use a substantially
wider margin than the current 6-second budget.
In `@test/regression.sh`:
- Line 198: Add reverse gate-list validation in test/manifestcheck.sh: compare
the manifest’s gate entries against the gate names in the regression.sh loop,
fail for any manifest gate missing from that loop, and reject duplicate gate
names in the manifest. Preserve existing validation while ensuring both
omissions and duplicates cause a failure.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d2f34fa-0213-414d-bcac-5edbfb6ea803
⛔ Files ignored due to path filters (1)
test/skillevalfix/prompts.tsvis excluded by!**/*.tsv
📒 Files selected for processing (24)
README.mddocs/COMMANDS.mddocs/EVALS.mdskills/ripwire-before-you-build/SKILL.mdskills/ripwire-fresh-eyes/SKILL.mdskills/ripwire-mcp/SKILL.mdskills/ripwire-navigate/SKILL.mdskills/ripwire-orient/SKILL.mdskills/ripwire-quality-bar/SKILL.mdsrc/cli.hsrc/clones.hsrc/gitmine.hsrc/main.cppsrc/query.hsrc/serialize.htest/churndecaycheck.shtest/evictioncheck.shtest/gateexitcheck.shtest/layerquerycheck.shtest/pargates.pytest/pargatescheck.shtest/regexbombcheck.shtest/regression.shtest/type3clonecheck.sh
| ``` | ||
|
|
||
| **Shaped by:** `--deps` | ||
| **Shaped by:** `--graph-query`, `--deps` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether --arch composes with --graph-query anywhere in the source.
rg -n 'graphQuery' src/main.cpp src/cli.h src/query.h 2>/dev/null
rg -n 'archRules' src/main.cpp src/query.h 2>/dev/nullRepository: redhat-et/ripwire
Length of output: 1665
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- documentation context ---'
sed -n '595,618p;1518,1533p' docs/COMMANDS.md
printf '%s\n' '--- graph-query execution ---'
sed -n '5400,5510p' src/main.cpp
printf '%s\n' '--- arch execution ---'
sed -n '3168,3310p' src/main.cpp
printf '%s\n' '--- modifier and command guards ---'
sed -n '2205,2240p;2338,2360p;10605,10645p;10835,10870p;10890,10920p;11260,11285p' src/main.cpp src/cli.h
printf '%s\n' '--- all relevant declarations and calls ---'
rg -n -C 3 'graphQuery|archRules|runArchViews|validateModifierGuards|graph-query|--arch' src/main.cpp src/cli.h src/query.hRepository: redhat-et/ripwire
Length of output: 50375
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- exact documentation locations ---'
rg -n -C 4 --fixed-strings 'It does NOT read a --arch=FILE rules file' docs/COMMANDS.md
rg -n -C 6 --fixed-strings '**Shaped by:** `--graph-query`, `--deps`' docs/COMMANDS.md
rg -n -C 3 -- '--arch=FILE' docs/COMMANDS.md | head -80
printf '%s\n' '--- dispatch implementation around precedence ---'
rg -n -C 8 'scanReportVerbPrecedence|runReport|runArch|runGraphQuery|winnerIsQueryFamily' src/main.cpp src/cli.h | head -240
printf '%s\n' '--- all source references to the claimed composition ---'
rg -n -C 2 -- '--arch.*graph-query|graph-query.*--arch|archRules.*graphQuery|graphQuery.*archRules' src docs test 2>/dev/null | head -200Repository: redhat-et/ripwire
Length of output: 18945
Correct the --arch “Shaped by” line.
runArchViews executes before runGraphQuery, so --arch and --graph-query do not compose. Remove --graph-query from the line. Check whether --deps also belongs there.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/COMMANDS.md` at line 1528, Update the “Shaped by” entry for --arch in
COMMANDS.md to remove --graph-query, since runArchViews executes before
runGraphQuery and they do not compose; also verify whether --deps actually
shapes --arch and remove it if it does not.
Comment on lines
+933
to
+946
| inline CloneGrouping groupClones( const IngestResult& ing, const std::vector<CloneGroup>& exact, const std::vector<CloneGroup>& gapped ) | ||
| { | ||
| CloneGrouping out; | ||
| out.gidOfGroup.assign( exact.size() + gapped.size(), 0u ); | ||
|
|
||
| // The detector's own universe — the SAME predicate findClones/findClonesType3 filter candidates with, so | ||
| // the denominator can never include code the numerator was never allowed to look at. | ||
| for( const Symbol& s : ing.symbols ) | ||
| { | ||
| if( ( s.kind == SymKind::Function || s.kind == SymKind::Method ) && s.endByte > s.sigEndByte ) | ||
| { | ||
| out.totalLoc += s.loc; | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Align totalLoc with the clone candidate set.
findClones and findClonesType3 exclude bodies below minTokens, but this loop includes them in totalLoc. Short functions can lower dup_pct even though the detector never considered them as clone candidates.
Pass the candidate universe or minTokens into this calculation. Count only symbols that pass the same filter.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/clones.h` around lines 933 - 946, The groupClones function currently
includes short function and method bodies in totalLoc even though findClones and
findClonesType3 exclude them. Pass the candidate universe or minTokens into
groupClones and apply the same minimum-token filter used by those detectors when
accumulating totalLoc, while preserving the existing symbol-kind and
body-validity checks.
Comment on lines
+1553
to
+1601
| bool anyCommit = false; | ||
| double curWeight = 0.0; // this commit's decayed weight | ||
| std::vector<std::uint32_t> cur; // this commit's resolved fileIds (dedup before tally) | ||
| const auto flush = [ & ]() | ||
| { | ||
| std::sort( cur.begin(), cur.end() ); | ||
| cur.erase( std::unique( cur.begin(), cur.end() ), cur.end() ); | ||
| if( cur.size() >= 1 && cur.size() <= maxFiles ) | ||
| { | ||
| for( std::uint32_t f : cur ) | ||
| { | ||
| weights[f] += curWeight; | ||
| } | ||
| } | ||
| cur.clear(); | ||
| }; | ||
| std::string s; | ||
| while( readByteSafeLine( pipe, s ) ) // F6: THE line reader, not a char[4096] a long path can be split across | ||
| { | ||
| while( !s.empty() && ( s.back() == '\n' || s.back() == '\r' ) ) | ||
| { | ||
| s.pop_back(); | ||
| } | ||
| if( s.rfind( "__C__", 0 ) == 0 ) // new commit marker: "__C__ <epoch>" | ||
| { | ||
| flush(); | ||
| anyCommit = true; | ||
| const std::int64_t epoch = ( s.size() > 6 ) ? std::strtoll( s.c_str() + 6, nullptr, 10 ) : 0; | ||
| const std::int64_t ageSec = ( epoch > 0 && headEpoch > epoch ) ? ( headEpoch - epoch ) : 0; // clamped: never > 1 | ||
| curWeight = std::pow( 0.5, ( double( ageSec ) / 86400.0 ) / kChurnDecayHalfLifeDays ); | ||
| continue; | ||
| } | ||
| if( s.empty() ) | ||
| { | ||
| continue; | ||
| } | ||
| const std::uint32_t f = resolveGitPath( byGitPath, s ); | ||
| if( f != UINT32_MAX ) | ||
| { | ||
| cur.push_back( f ); | ||
| } | ||
| } | ||
| flush(); | ||
| pclose( pipe ); | ||
| if( outAnyHistory ) | ||
| { | ||
| *outAnyHistory = anyCommit; | ||
| } | ||
| return weights; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
anyCommit disclosure does not match weights contribution.
anyCommit is set on every __C__ marker line, regardless of whether that commit's files pass the cur.size() >= 1 && cur.size() <= maxFiles check in flush(). churnTeleport's sibling gates its evidence flag on !sets.empty(), which only counts commits that passed the equivalent check.
In an all-merge-bomb history (every commit exceeds maxFiles), weights stays all-zero and the computed prior is numerically uniform either way, but outAnyHistory reports true. The caller then skips the "no churn evidence" disclosure (DEGRADED_PATH_ALERT, the stderr note, and the window-label suffix), even though the ranking is silently the structural one.
Track contribution inside flush() instead of marker-sightings.
🐛 Proposed fix
- bool anyCommit = false;
+ bool anyContributed = false;
double curWeight = 0.0; // this commit's decayed weight
std::vector<std::uint32_t> cur; // this commit's resolved fileIds (dedup before tally)
const auto flush = [ & ]()
{
std::sort( cur.begin(), cur.end() );
cur.erase( std::unique( cur.begin(), cur.end() ), cur.end() );
if( cur.size() >= 1 && cur.size() <= maxFiles )
{
+ anyContributed = true;
for( std::uint32_t f : cur )
{
weights[f] += curWeight;
}
}
cur.clear();
};
std::string s;
while( readByteSafeLine( pipe, s ) )
{
while( !s.empty() && ( s.back() == '\n' || s.back() == '\r' ) )
{
s.pop_back();
}
if( s.rfind( "__C__", 0 ) == 0 )
{
flush();
- anyCommit = true;
const std::int64_t epoch = ( s.size() > 6 ) ? std::strtoll( s.c_str() + 6, nullptr, 10 ) : 0;
const std::int64_t ageSec = ( epoch > 0 && headEpoch > epoch ) ? ( headEpoch - epoch ) : 0;
curWeight = std::pow( 0.5, ( double( ageSec ) / 86400.0 ) / kChurnDecayHalfLifeDays );
continue;
}
if( s.empty() )
{
continue;
}
const std::uint32_t f = resolveGitPath( byGitPath, s );
if( f != UINT32_MAX )
{
cur.push_back( f );
}
}
flush();
pclose( pipe );
if( outAnyHistory )
{
- *outAnyHistory = anyCommit;
+ *outAnyHistory = anyContributed;
}
return weights;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| bool anyCommit = false; | |
| double curWeight = 0.0; // this commit's decayed weight | |
| std::vector<std::uint32_t> cur; // this commit's resolved fileIds (dedup before tally) | |
| const auto flush = [ & ]() | |
| { | |
| std::sort( cur.begin(), cur.end() ); | |
| cur.erase( std::unique( cur.begin(), cur.end() ), cur.end() ); | |
| if( cur.size() >= 1 && cur.size() <= maxFiles ) | |
| { | |
| for( std::uint32_t f : cur ) | |
| { | |
| weights[f] += curWeight; | |
| } | |
| } | |
| cur.clear(); | |
| }; | |
| std::string s; | |
| while( readByteSafeLine( pipe, s ) ) // F6: THE line reader, not a char[4096] a long path can be split across | |
| { | |
| while( !s.empty() && ( s.back() == '\n' || s.back() == '\r' ) ) | |
| { | |
| s.pop_back(); | |
| } | |
| if( s.rfind( "__C__", 0 ) == 0 ) // new commit marker: "__C__ <epoch>" | |
| { | |
| flush(); | |
| anyCommit = true; | |
| const std::int64_t epoch = ( s.size() > 6 ) ? std::strtoll( s.c_str() + 6, nullptr, 10 ) : 0; | |
| const std::int64_t ageSec = ( epoch > 0 && headEpoch > epoch ) ? ( headEpoch - epoch ) : 0; // clamped: never > 1 | |
| curWeight = std::pow( 0.5, ( double( ageSec ) / 86400.0 ) / kChurnDecayHalfLifeDays ); | |
| continue; | |
| } | |
| if( s.empty() ) | |
| { | |
| continue; | |
| } | |
| const std::uint32_t f = resolveGitPath( byGitPath, s ); | |
| if( f != UINT32_MAX ) | |
| { | |
| cur.push_back( f ); | |
| } | |
| } | |
| flush(); | |
| pclose( pipe ); | |
| if( outAnyHistory ) | |
| { | |
| *outAnyHistory = anyCommit; | |
| } | |
| return weights; | |
| bool anyContributed = false; | |
| double curWeight = 0.0; // this commit's decayed weight | |
| std::vector<std::uint32_t> cur; // this commit's resolved fileIds (dedup before tally) | |
| const auto flush = [ & ]() | |
| { | |
| std::sort( cur.begin(), cur.end() ); | |
| cur.erase( std::unique( cur.begin(), cur.end() ), cur.end() ); | |
| if( cur.size() >= 1 && cur.size() <= maxFiles ) | |
| { | |
| anyContributed = true; | |
| for( std::uint32_t f : cur ) | |
| { | |
| weights[f] += curWeight; | |
| } | |
| } | |
| cur.clear(); | |
| }; | |
| std::string s; | |
| while( readByteSafeLine( pipe, s ) ) // F6: THE line reader, not a char[4096] a long path can be split across | |
| { | |
| while( !s.empty() && ( s.back() == '\n' || s.back() == '\r' ) ) | |
| { | |
| s.pop_back(); | |
| } | |
| if( s.rfind( "__C__", 0 ) == 0 ) // new commit marker: "__C__ <epoch>" | |
| { | |
| flush(); | |
| const std::int64_t epoch = ( s.size() > 6 ) ? std::strtoll( s.c_str() + 6, nullptr, 10 ) : 0; | |
| const std::int64_t ageSec = ( epoch > 0 && headEpoch > epoch ) ? ( headEpoch - epoch ) : 0; // clamped: never > 1 | |
| curWeight = std::pow( 0.5, ( double( ageSec ) / 86400.0 ) / kChurnDecayHalfLifeDays ); | |
| continue; | |
| } | |
| if( s.empty() ) | |
| { | |
| continue; | |
| } | |
| const std::uint32_t f = resolveGitPath( byGitPath, s ); | |
| if( f != UINT32_MAX ) | |
| { | |
| cur.push_back( f ); | |
| } | |
| } | |
| flush(); | |
| pclose( pipe ); | |
| if( outAnyHistory ) | |
| { | |
| *outAnyHistory = anyContributed; | |
| } | |
| return weights; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/gitmine.h` around lines 1553 - 1601, Update the churn history evidence
tracking so outAnyHistory reflects only commits that contribute weights: move
the anyCommit=true assignment from the __C__ marker handling into flush(),
setting it only when the deduplicated cur collection passes the existing
1..maxFiles gate. Keep marker parsing and weight accumulation unchanged.
Comment on lines
+212
to
+217
| if command -v xmllint >/dev/null 2>&1; then | ||
| printf '%s' "$G" | xmllint --noout - 2>"$W2/xl" && p2ok "arm K2: extended --clones output is well-formed XML" \ | ||
| || { p2no "arm K2: xmllint rejected the extended output"; sed 's/^/ /' "$W2/xl" | head -3; } | ||
| else | ||
| p2no "arm K2: xmllint missing — cannot verify well-formedness (install libxml2-utils)" | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not fail the gate when optional xmllint is absent.
Line 216 calls p2no, so the final check at line 219 fails the test on systems without xmllint. Report this case as skipped, or make xmllint a declared required dependency.
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 213-213: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/type3clonecheck.sh` around lines 212 - 217, Update the
xmllint-unavailable branch in the extended clones XML validation check to mark
the verification as skipped rather than calling p2no, so the final gate does not
fail when the optional dependency is absent; keep the existing failure behavior
when xmllint is present but rejects the output.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three lanes integrated on v0.3.0's main.
Consensus content-gaps round (ACCEPTED 9/9) — six description edits adding missing FACTS (tool-health → mcp/orient with
--doctor/--skippedby name,--connectN-way relate → navigate,--lego→ before-you-build, the fresh-eyes↔quality-bar boundary sentences) + three two-rater-consensus relabels with a RELABEL log. Pre-registered band [8,9] on a fresh sealed two-rater pass; measured 9/9, unsupported-flags 8/9 → 1/9; all bm25 floors held. Registration + result in EVALS.P0 fleet —
--rank-by=churn-decay(half-life 90d disclosed in the legend, clock anchored to HEAD's commit epoch — proven identical across repos dated five years apart);layer(SET,NAME)in--graph-query(reads the same builtinLayer the map prints; refuses loudly on unknown layers or unlayered trees); Type-3 clone grouping via union-find withgid=per row andclone_groups=/dup_loc=/dup_pct=floors (counting choice disclosed: a 3-clone group counts its lines twice). Red-first: 10 + 10 + 8 arms.Hardening — pargates.py per-gate timeout budgets (declared table; a red names its own budget; cppbench/regexbomb get honest 1200s) with the regexbomb killed-scan misreport fixed (was OVER-REFUSED, now TIMEOUT/KILLED, repro'd red-first); evictioncheck arms for the
.cachefamily and the locks/-never-touched invariant, mutation-catch verified both ways.Integration: gate loop set-unioned 385/386 → 387; all three EVALS counts recomputed; full plain suite ALL PASS on the union; determinism ×3; xmllint; quality-delta gating=0.
🤖 Generated with Claude Code