Implement lempel-ziv-trajectory-complexity grader (Tier 1, rank 11) - #56972
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
Lean already. Ship. Warning Firewall blocked 4 domainsThe following domains were blocked by the firewall during workflow execution:
[!TIP] tools:
github:
mode: gh-proxySee GitHub Tools for more information on To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"
- "api.github.com"
- "chatgpt.com"
- "github.com"See Network Configuration for more information.
|
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
The new grader's LZ76 parser is not trustworthy yet: the terminal phrase-count logic overstates complexity on low-entropy traces, so the metric is wrong exactly where it is supposed to be most informative.
Blocking theme
- The implementation unconditionally increments the final phrase count when the scan ends with
subSeqLen !== 1, which produces inflated complexity for degenerate repeated-symbol sequences and likely other tail cases. - This needs targeted regression tests around constant traces, short repeating motifs, and short unique traces before we can treat the metric as a reliable signal.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 10.9 AIC · ⌖ 7.42 AIC · ⊞ 7.2K
Comment /review to run again
| complexity += 1; | ||
| prefixLen += maxSubSeqLen; | ||
| pointer = 0; | ||
| maxSubSeqLen = 1; |
There was a problem hiding this comment.
This implementation overcounts LZ76 complexity on constant or single-phrase tails because if (subSeqLen !== 1) complexity += 1 blindly adds a phrase after the scan terminates, which makes a uniform sequence like ['a','a',...] report rawComplexity=2 instead of 1 and inflates the normalized score for the exact low-complexity traces this grader is supposed to catch.
💡 Why this blocks merge
The tail handling here is the fragile part of incremental LZ76 parsing: reaching the end of the scan with subSeqLen > 1 does not always mean a new phrase was discovered. In the degenerate repeated-symbol case, the loop exits only because the candidate extension hit the sequence boundary, but the parser never proved a new phrase boundary. Adding one unconditionally shifts the baseline upward and makes the metric materially wrong on the strongest "agent is stuck in a trivial loop" signal.
A minimal regression case is a trace where every canonical symbol is identical; this code currently yields complexity=2, even though there is only one phrase.
Please either switch to a known-correct LZ76 parsing implementation/test oracle or add explicit tests for edge cases like:
['a','a','a','a'] // should stay at the minimum complexity
['a','b','a','b','a','b'] // repeating motif
['a','b','c','d'] // maximal complexity for short unique traceand only increment the final phrase count when the termination state actually represents a completed new phrase.
There was a problem hiding this comment.
🔵 Needs a closer look
Protected workflow logic requires human scrutiny, and two scoring correctness issues remain unresolved.
Pull request overview
Adds a shared LZ76-based trajectory grader, but event qualification and not-applicable handling introduce correctness issues.
Changes:
- Adds event projection, LZ76 parsing, and normalized scoring.
- Marks the Tier 1 grader implemented.
File summaries
| File | Description |
|---|---|
.github/workflows/shared/graders/lempel-ziv-trajectory-complexity.md |
Implements the grader. |
.github/workflows/shared/graders/README.md |
Updates catalog status. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| if (!isRecord(event) || typeof event.kind !== "string" || event.kind === "") return null; | ||
| const index = Number(event.index); | ||
| const order = Number.isFinite(index) ? index : position; | ||
| const symbol = event.kind === "tool_call" && typeof event.ref === "string" && event.ref !== "" ? `tool_call:${event.ref}` : event.kind; |
| const sequence = ordered.map(entry => entry.symbol); | ||
| const n = sequence.length; | ||
| if (n < 2) { | ||
| return { value: 0, unit: "ratio", passed: null, message: "not applicable: fewer than two events" }; |
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /codebase-design — commenting with suggestions; no blocking issues, but the algorithm correctness deserves validation.
📋 Key Themes & Highlights
Key Themes
passed: nullinconsistency (line 56): thealphabetSize < 2early return usesdetailsinstead ofpassed: null+message, unlike then < 2branch — the framework may treat these differently.- LZ76 variant ambiguity (line 86): the
maxSubSeqLentracking is a non-obvious variant of the algorithm; a citation or comment would help future reviewers validate without reverse-engineering. - Code duplication (line 26): the
isRecordhelper andcandidateslookup block are verbatim copies fromevent-entropy-rate.md— consider a sharedhelpers.extractEventSequenceif the framework allows it. - Small-n normalization (line 113): for small sequences the asymptotic bound can over-normalize; the
helpers.clampcall is the right safety net but should be documented as intentional.
Positive Highlights
- ✅ Clean separation from
event-entropy-rate: correctly scoped to whole-sequence compressibility, not first-order statistics - ✅ Defensive IR lookup (multiple
trajectoryIR/trajectoryIr/irvariants) matches the existing convention - ✅ Good use of
helpers.clampto enforce[0, 1]output bounds - ✅ Well-written inline comments explaining the algorithm and its relationship to the recurrence-* family
- ✅ PR description includes worked examples with expected outputs
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet46 · 70.8 AIC · ⌖ 11 AIC · ⊞ 7.6K
Comment /matt to run again
| return { symbol, order }; | ||
| }).filter(entry => entry !== null).sort((a, b) => a.order - b.order); | ||
|
|
||
| const sequence = ordered.map(entry => entry.symbol); |
There was a problem hiding this comment.
[/tdd] The alphabetSize < 2 early return omits passed: null, while the n < 2 branch on line 52 includes it. If the grader framework uses passed: null as a signal for "not applicable" (distinct from a degenerate-but-valid score of 0), this inconsistency could cause routing issues downstream.
💡 Suggested fix
if (alphabetSize < 2) {
return { value: 0, unit: "ratio", passed: null, message: `not applicable: single-symbol alphabet (events=${n})` };
}Aligning both early returns to use passed: null + message (rather than details) makes the "not applicable" intent unambiguous to the grader framework, consistent with event-entropy-rate.md line 52.
@copilot please address this.
| if (pointer === prefixLen) { | ||
| complexity += 1; | ||
| prefixLen += maxSubSeqLen; | ||
| pointer = 0; |
There was a problem hiding this comment.
[/tdd] The maxSubSeqLen tracking in the inner loop is a non-obvious LZ76 variant: when a mismatch occurs, subSeqLen resets to 1 but maxSubSeqLen preserves the best run, and the phrase advances by maxSubSeqLen. A comment citing which specific variant/reference this matches would help reviewers validate correctness.
💡 Why this matters: traced example for ["a","b","a","b"]
n=4, complexity=1, prefixLen=1, subSeqLen=1, maxSubSeqLen=1, pointer=0
Step 1: seq[0]="a" vs seq[1]="b"? NO
pointer→1 == prefixLen(1) → complexity=2, prefixLen=2, reset
Step 2: seq[0]="a" vs seq[2]="a"? YES → subSeqLen=2
Step 3: seq[1]="b" vs seq[3]="b"? YES → subSeqLen=3
prefixLen+subSeqLen=5 > 4 → exit
subSeqLen(3)!=1 → complexity=3
normalized = 3/4 = 0.75
Is 0.75 the expected value for period-2 ["a","b","a","b"]? Adding a short comment like // variant: Lempel-Ziv '76 copy/insert with maximal-copy phrase tracking and a link to the original paper would let the next reader verify this without reverse-engineering the loop.
@copilot please address this.
| min: 0.0 | ||
| max: 1.0 | ||
| script: | | ||
| const isRecord = value => value !== null && typeof value === "object" && !Array.isArray(value); |
There was a problem hiding this comment.
[/codebase-design] The isRecord helper and the candidates/event-extraction loop are verbatim copies from event-entropy-rate.md. Any future fix to the IR lookup strategy (e.g. adding a new key variant) must be applied to both files independently — a shotgun-surgery smell that will grow with each new event-sequence grader.
💡 Mitigation options
- If
helpers(already used forhelpers.clamp) is an extensible namespace, exposehelpers.extractEventSequence(trace)there and call it from both graders. - If not, a comment like
// Canonical IR lookup — keep in sync with event-entropy-rate.mdat minimum flags the coupling to future authors.
@copilot please address this.
| lempel-ziv-trajectory-complexity computes the normalized LZ76 (Kaspar & | ||
| Schuster) incremental-parsing complexity of the ordered canonical | ||
| event-symbol sequence built purely from the Trajectory IR's events[] (no | ||
| states, provenance, or objectives required). It measures whole-sequence |
There was a problem hiding this comment.
[/tdd] upperBound = n / log_b(n) is undefined when n = 1 (log of 1 = 0 → division by zero), and potentially very large when n is small (e.g. n=2, b=2 → upperBound=2/1=2, so normalized=1/2=0.5 which seems fine, but n=2, b=3 → log_3(2)≈0.63 → upperBound≈3.17 → complexity 2 / 3.17 → 0.63). The n < 2 guard prevents the n=1 case, but documenting or testing that helpers.clamp is the correct safety valve for small-n over-normalization would make the intention explicit.
💡 Suggestion
Add a comment above the upperBound line:
// For small n the asymptotic bound underestimates; clamp ensures value stays in [0,1].
const upperBound = n / (Math.log(n) / Math.log(alphabetSize));
const normalized = helpers.clamp(complexity / upperBound, 0, 1);And consider adding a test vector for n=2 with both b=2 and b=alphabet-size to confirm the clamp behaves as intended.
@copilot please address this.
There was a problem hiding this comment.
Review: lempel-ziv-trajectory-complexity grader
Overall: The implementation is correct and well-structured, closely following the conventions established by event-entropy-rate.md. The LZ76 (Kaspar & Schuster) parsing loop logic is sound and produces correct phrase counts on all tested cases.
One non-blocking issue found:
The YAML header comment and inline // Normalize comment both claim that dividing by n / log_b(n) keeps the value "in [0, 1]". However, this asymptotic upper bound is only guaranteed for large n. For short sequences (≥ 7 symbols), the raw LZ76 phrase count can exceed this bound (tested: ABAABBA → c=5, upperBound≈2.49, ratio ≈ 2.0). The helpers.clamp call is therefore load-bearing, not merely defensive. The misleading comment should be corrected (see inline comment).
Everything else — candidate resolution, symbol construction, the alphabetSize < 2 early exit, the sorting by event.index, the details field format — matches the established pattern and is correct.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet46 · 76 AIC · ⌖ 10.8 AIC · ⊞ 6.2K
| if (subSeqLen !== 1) complexity += 1; | ||
|
|
||
| // Normalize by the theoretical asymptotic upper bound n / log_b(n) | ||
| // (b = alphabet size) so the value stays in [0, 1] and remains |
There was a problem hiding this comment.
The comments claim that dividing by n / log_b(n) "stay[s] in [0, 1]", but the LZ76 asymptotic upper bound only holds for large n. For short sequences (as few as 7 symbols) the raw complexity can exceed this bound — e.g., ABAABBA yields c=5 vs upperBound≈2.49, a ratio of ~2.0. This means helpers.clamp is load-bearing and not merely defensive.
The code is correct because helpers.clamp does enforce [0, 1], but the comment misleads readers into thinking the formula alone guarantees the range. Please correct:
- // Normalize by the theoretical asymptotic upper bound n / log_b(n)
- // (b = alphabet size) so the value stays in [0, 1] and remains
- // comparable across traces of different lengths/alphabets.
+ // Normalize by the theoretical asymptotic upper bound n / log_b(n)
+ // (b = alphabet size) to make the value comparable across traces of
+ // different lengths/alphabets. For short sequences the phrase count
+ // can exceed this asymptotic bound, so helpers.clamp enforces [0, 1].The same correction applies to the YAML header comment that also says "to stay in [0, 1]".
@copilot please address this.
|
🎉 This pull request is included in a new release. Release: |
Adds the
lempel-ziv-trajectory-complexitygrader to the shared graders catalog, the next unimplemented Tier 1 grader (rank 11 of 25).What it measures
LZ76 (Kaspar & Schuster) incremental-parsing complexity of the ordered canonical event-symbol sequence built from the Trajectory IR's
events[]. It counts distinct phrases extracted by the copy/insert parsing rule and normalizes by the asymptotic upper boundn / log_b(n)(b = alphabet size) to stay in[0, 1]and be comparable across traces of different lengths/alphabets. Unlikeevent-entropy-rate(first-order, previous-symbol predictability), it detects compressibility from arbitrary-length repeated motifs anywhere earlier in the sequence, with no Markov assumption and no need for canonical states (unlike therecurrence-*family).Changes
.github/workflows/shared/graders/lempel-ziv-trajectory-complexity.md(new) — grader fragment following the same projection pattern asevent-entropy-rate.md: builds the qualified event-symbol sequence (tool_call events qualified byref), runs LZ76 parsing, normalizes the raw phrase count, and returnsnot applicablefor fewer than two events..github/workflows/shared/graders/README.md— status flipped fromNot startedtoImplementedfor rank 11.Example
Low values flag traces dominated by a few repeated motifs (likely stuck in a loop); values near 1 flag near-incompressible, structurally diverse traces.