feat: deterministic aggregator + wet-run feedback fixes#5
Merged
Conversation
Six improvements from the wet-run report: - Deterministic aggregator at skills/dishonest-code-audit/lib/aggregate.py: parses both source reports, dedups by (file, line) with Jaccard fuzzy fallback on User-visible lie when line is unknown, max-merges severity (recording disagreement), applies caller-supplied known_clean_surfaces, and emits AGGREGATE.json plus a Markdown skeleton with LLM_FILL placeholders for the narrative parts. Fails loud on malformed input (path:line in the error). Wet-run motivation: orchestrator LLM was miscounting. - stub-audit: try/finally-no-catch candidate type added to the TypeScript profile and a Phase-3 judgment step with worked example added to SKILL.md. Framed as deliberate double-coverage with silent-failure-hunter; redundancy is the point, not a bug. - known_clean_surfaces promoted to a first-class structured parameter in dishonest-code-audit step 1, passed into both Task prompts, with an aggregator-side MEDIUM-to-LOW demotion rule that requires an annotation when applied. - Cross-audit gaps section made REQUIRED in the combined-report template so single-source HIGH findings get explicit cross-coverage judgment (tuning signal for profile improvements). - README updated: "Two specialists, near-disjoint surfaces" subsection frames the specialists as complementary (the deliberate redundancy zone is small), plus a short note on the prompt-injection guard's upfront-cost vs contingent-value tradeoff. - CI: tests/run-aggregator-tests.sh with six fixture cases (exact overlap, fuzzy overlap, fuzzy miss, severity disagreement, known-clean reclassification, malformed-block fail-loud) wired into .github/workflows/ci.yml. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds a deterministic aggregation layer and regression coverage for the dishonest-code-audit plugin, incorporating wet-run feedback around deduping, known-clean surfaces, and cross-audit tuning signals.
Changes:
- Introduces a deterministic Python aggregator that parses both specialist reports, deduplicates/merges, applies known-clean reclassification, and emits
AGGREGATE.json+ a combined Markdown skeleton. - Expands
stub-auditguidance and TS profile sweeps to includetry/finally(no-catch) “silent success” candidates. - Adds an aggregator regression harness with fixtures and wires it into CI.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/ci.yml |
Runs shellcheck on the new harness and executes aggregator regression tests in CI. |
README.md |
Documents the “two specialists, near-disjoint surfaces” framing and prompt-injection guard tradeoff. |
skills/dishonest-code-audit/SKILL.md |
Updates orchestrator procedure to use the deterministic aggregator and formalizes known_clean_surfaces + required cross-audit gaps section. |
skills/dishonest-code-audit/lib/aggregate.py |
New deterministic aggregator implementation producing AGGREGATE.json and a combined Markdown skeleton. |
skills/stub-audit/SKILL.md |
Adds Phase 3 judgment guidance + example for try/finally without catch. |
skills/stub-audit/profiles/typescript.md |
Adds a TS/JS ripgrep sweep to surface try { … throw … } finally { … } candidates. |
tests/run-aggregator-tests.sh |
New regression harness that runs aggregator across fixtures and asserts partial JSON expectations. |
tests/fixtures/aggregator/case-01-exact-overlap/* |
Fixture covering exact dedup overlap behavior. |
tests/fixtures/aggregator/case-02-fuzzy-overlap/* |
Fixture covering fuzzy dedup (unknown line) behavior. |
tests/fixtures/aggregator/case-03-fuzzy-miss/* |
Fixture ensuring fuzzy match doesn’t over-dedup across different findings. |
tests/fixtures/aggregator/case-04-severity-disagreement/* |
Fixture covering max-severity merge + disagreement recording. |
tests/fixtures/aggregator/case-05-known-clean/* |
Fixture covering known-clean surface reclassification behavior. |
tests/fixtures/aggregator/case-06-malformed/* |
Fixture ensuring malformed blocks fail loud (non-zero + file:line in stderr). |
Comments suppressed due to low confidence (2)
skills/dishonest-code-audit/lib/aggregate.py:540
intentional_totalonly counts severities in {"FALSE-POSITIVE", "INTENTIONAL"}. If a source report emits the unified labelFALSE-POSITIVE / INTENTIONAL, it won’t be counted as intentional in the summary counts. Include the unified label in this set (or normalize it) so counts match the documented severity vocabulary.
"high_total": count_merged_sev("HIGH"),
"medium_total": count_merged_sev("MEDIUM"),
"low_total": count_merged_sev("LOW"),
"intentional_total": sum(1 for m in merged if m.severity in {"FALSE-POSITIVE", "INTENTIONAL"}),
}
skills/dishonest-code-audit/SKILL.md:118
- This section instructs specialists to emit
Severity: FALSE-POSITIVE / INTENTIONAL, but a few lines later the “exact severity vocabulary” and the example schema omit that combined label (they only list FALSE-POSITIVE and INTENTIONAL). Align the vocabulary + schema with the unified label (or remove the instruction to emit it) to avoid generating reports the aggregator can’t parse/aggregate consistently.
known_clean_surfaces (from caller — treat each as already-verified intact):
<pass the structured list verbatim, or "none" if empty>
For any candidate finding whose `file:symbol` matches an entry above, emit a structured block classified `FALSE-POSITIVE / INTENTIONAL` with `Recommended fix: none — marked clean by caller: <reason>`. Do not silently drop these; the orchestrator needs the entries to confirm the surfaces stayed clean.
Use this exact severity vocabulary: HIGH | MEDIUM | LOW | FALSE-POSITIVE | INTENTIONAL. Do not invent other labels.
Emit every HIGH and MEDIUM finding as a structured block:
### Finding ID: SAFE-001
Severity: HIGH | MEDIUM | LOW | FALSE-POSITIVE | INTENTIONAL
File: path/to/file.tsx
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c273d95235
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…field parsing, M1 SKILL.md snippet) - B1: known_clean reclassification now tracks pre-reclass severity and the rendered equation includes a `reclassified by known_clean_surfaces` term, so `1 safe + 1 mock - 1 overlap - 1 reclassified = 0` reads honestly. build_counts derives overlaps against pre-reclass severity. New case-07-reclass-overlap fixture asserts the equation reconciles. - B2: unknown top-level field keys are no longer silently captured into extra_fields. The parser only treats a `Capital: word` line as a new field when the key is in the allowed set (REQUIRED + the four combined-report optional fields). A continuation that begins with `Status:` etc. is now preserved as part of the previous field rather than truncating it. New case-08-strict-unknown-key fixture asserts the full text is preserved. - M1: SKILL.md step 5 no longer ships a broken `$(realpath "$0")` snippet. Replaced with three concrete location strategies (loader-supplied env var, ~/.claude/plugins scan, Python walker) that work in actual runtime contexts. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Five fixes from Copilot + Codex automated review on PR #5: - KeyError on unified `FALSE-POSITIVE / INTENTIONAL` label (P1, both bots): normalize_severity() now canonicalizes the unified label to INTENTIONAL so the render_markdown bucket and intentional_total set never miss-key. New case-09-unified-fp-label fixture pins the behavior. - Silent skip of malformed known-clean entries (Copilot): parse_known_clean() now raises with file:line on any non-blank, non-comment line that doesn't match the expected form. Matches the file's "fail loud" posture. New case-12-malformed-known-clean fixture asserts the failure mode. - Blank line inside a block prematurely ends parsing (Codex P2): the parser now only terminates a block at the next `### Finding ID:` header or EOF. Blank-line visual spacing between fields is tolerated. New case-10-blank-lines-in-block fixture asserts the block parses cleanly. - Fuzzy match picks first candidate over threshold, not best (Codex P2): deduplicate() now ranks fuzzy candidates by Jaccard score descending and takes the highest. Prevents misrouting when one file has multiple unknown-line findings. New case-11-fuzzy-best-match fixture pins the higher-scoring candidate as the partner. - Unused `import os` removed (Copilot, cosmetic). Test harness change: case-06's hardcoded fail-loud branch generalized into a `must_fail` declaration in expected.json, so future fail-loud cases (like case-12) don't need a hardcoded name in the harness. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three files per https://docs.github.com/en/copilot/how-tos/copilot-on-github/customize-copilot/add-custom-instructions/add-repository-instructions: - .github/copilot-instructions.md — repo-wide guidance. Calls out load-bearing patterns the wet-run review showed Copilot misreads (bash redirection order in test harnesses, prompt-injection guard, fail-loud posture in the aggregator, unified FALSE-POSITIVE/INTENTIONAL severity label, npx --yes for zero-install toolchain). - .github/instructions/fixtures.instructions.md (applyTo: tests/fixtures/**) — fixtures are planted inputs to the harness, not production code. Tells Copilot what counts as a real concern (orphan plants, non-reconciling expected.json) vs noise (Hebrew strings, tiny files, "broken" code that is the detection target). - .github/instructions/skill-md.instructions.md (applyTo: skills/**/*.md) — SKILL.md and profile files are LLM-runtime instructions, not human docs. Critique against runtime-correctness criteria (ambiguity, missing graceful- degradation, schema mismatch) instead of human-doc clarity. All three files under the 2-page constraint. Each item is grounded in a specific bot misread from the wet run. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Four issues from Copilot's second-pass review on PR #5: - normalize_path didn't honor the SKILL.md spec for case-insensitive filesystems. New --case-insensitive-paths flag casefolds File: values during dedup. Off by default because Linux is case-sensitive and Foo/x.tsx and foo/x.tsx CAN be distinct files there. New case-15-case-insensitive-paths fixture verifies cross-case merge under the flag. - deduplicate() silently overwrote ms_by_key on duplicate (file, line) within a source, making partner-pick order-dependent. Now raises ValueError with file:line:finding_id for both safe-fail and mock-stub. Within-source dedup is the specialist's responsibility. New case-13-duplicate-within-source fixture asserts the failure mode. - --known-clean-surfaces with a non-existent path was silently ignored. Now exits 2 with the missing path named in stderr. New case-14-missing-known-clean fixture pins the behavior; a corresponding harness extension (known-clean-arg.txt) lets fixtures pass a literal path string rather than a file in the fixture dir. - run-aggregator-tests.sh header comment claimed assertions were "recursive"; rewritten to accurately enumerate which structures assert_subset checks. Harness additions: - known-clean-arg.txt: override --known-clean-surfaces with a literal path (for fail-loud cases that need a non-existent path). - extra-args.txt: append arbitrary CLI flags to the aggregator invocation (e.g. --case-insensitive-paths). - grep -qE -- "$pattern" in must-fail check; pattern strings starting with `--` were being parsed as grep options. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three issues from Copilot's third-pass review on PR #5: - build_counts() didn't reconcile under severity disagreement. With safe-fail HIGH + mock-stub MEDIUM merging to HIGH, the rendered equation read `1 + 0 - 1 = 1` (incoherent math) and MEDIUM read `0 + 1 - 0 = 0`. Refactored so per-source counts are computed against EFFECTIVE merged severity, not raw source severity. The mock-stub's contribution to a HIGH merge now counts as mock_high, and its raw MEDIUM doesn't appear in the MEDIUM equation because the finding got promoted. case-04 expected.json updated to lock the reconciliation in. Same logic preserves the case-07 reclassification math. - Known-clean surfaces section claimed verification for caller entries that never matched any finding. apply_known_clean() now returns the set of matched indices; render_markdown() splits known-clean entries into "verified intact in this run" (matched) and "not observed in this run" (unmatched, with a paragraph explaining the ambiguity). AGGREGATE.json records `matched: bool` per entry. New case-16-unmatched-known-clean fixture asserts both paths. - apply_known_clean() didn't normalize caller-supplied paths. A caller passing `./src/Foo.tsx` would mismatch the aggregator's normalized `src/Foo.tsx`. Now runs known-clean paths through normalize_path() with the same case_insensitive_paths flag. New case-17-known-clean-path-normalize fixture asserts a leading-./ entry still matches. Harness extension: assert_subset() learned to check known_clean_surfaces entries by file with arbitrary key/value assertions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- "Why this exists" now covers both wet runs. Second-wet-run section names the two dominant patterns (HostControlRoom 7-handler try/finally-no-catch, state-endpoint missing-error-field) and ties the cross-audit-gaps signal to the new stub-audit pattern that ships in this PR. - New "Deterministic aggregator" subsection under Output explains the arithmetic invariant, fail-loud properties, --known-clean-surfaces, and --case-insensitive-paths. - New example 1a shows the known_clean_surfaces flow with matched-vs-not-observed. - AGGREGATE.json now listed as an output alongside the three Markdown files. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
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.
Summary
Six fixes from the wet-run feedback report on the dishonest-code-audit plugin:
skills/dishonest-code-audit/lib/aggregate.py: parses both source reports, dedups by(file, line)with Jaccard fuzzy fallback onUser-visible liewhen line is unknown, max-merges severity (recording disagreement), applies caller-suppliedknown_clean_surfaces, and emitsAGGREGATE.jsonplus a Markdown skeleton withLLM_FILLplaceholders. Fails loud on malformed input. Motivation: orchestrator LLM was miscounting.skills/stub-audit/SKILL.md. Framed as deliberate double-coverage with silent-failure-hunter; redundancy is the point.known_clean_surfacesfirst-class: promoted to a structured parameter indishonest-code-auditstep 1, passed into both Task prompts, with an aggregator-side MEDIUM-to-LOW demotion rule that requires an annotation when applied.tests/run-aggregator-tests.shcovering six aggregator cases (exact overlap, fuzzy overlap, fuzzy miss, severity disagreement, known-clean reclassification, malformed-block fail-loud) wired into.github/workflows/ci.yml.Test plan
bash tests/run-fixtures.sh— 5 fixtures, 9 expected findings, 0 failuresbash tests/run-aggregator-tests.sh— 6 cases, 0 failuresbash -n)Generated with Claude Code