ci(#5509): add harness-eval static analysis for agent configurations - #5510
ci(#5509): add harness-eval static analysis for agent configurations#5510Benkapner wants to merge 1 commit into
Conversation
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
1 similar comment
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
PR Summary by QodoCI: add harness-eval checks for agent configuration changes
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Site previewPreview: https://455ddefe-site.fullsend-ai.workers.dev Commit: |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
Agent files will disappear soon from this repository, is this still valid with that fact? |
|
@rh-hemartin and this repo will not have any harness files at all? agents, skills, commands, claude md hooks? if not so its not relevant but maybe in the new/other repo they will be? |
No, they were moved to github.com/fullsend-ai/agents. We will have skills, commands and claude.md hooks here, but for our development usage, not for agents. |
|
the main thing this tool does is lint + security checks if the user has committed any change to one of those harness files so could be relevant both here and the fullsend-ai/agents. wont hurt to add it to the CI. also it connects to docs/problems/trustworthiness-evidence.md @rh-hemartin |
|
Ah, for a moment I was confused as well regarding the concept of harness, for us is a specific thing. Then it could be useful here, yes. You need to pass |
rh-hemartin
left a comment
rh-hemartin
left a comment
There was a problem hiding this comment.
Incorporate this as a step in the current lint.yaml workflow, as it contains all the CI checks. The file filter can be done within a step, or just run it against everything. Optionally, since this does not require LLM, you could add it as a pre-commit (however this is more involved than the current setup, so don't worry, maybe a followup).
|
moved harness-eval into lint.yml as a step in the test job instead of a separate workflow. uses the checkout v7.0.1 already pinned there. will look into pre-commit integration as a follow-up. |
|
You need to fix all the commits to |
4d397b2 to
310f489
Compare
|
squashed into a single commit with ci(#5509) prefix. should pass commit-lint now. |
|
Hey! Would you able to fix the problems raised by the tool? |
310f489 to
cb21805
Compare
|
they were false positives of broken references. i fixed all of them (generic solution) in my repo and released harness-eval v6.1.1 with better path resolution for broken-references (checks project root, skill scripts/ subdir, skips code blocks, skips bare filenames). bumped to ==6.1.1 in this PR. the step now passes with 0 errors on this repo. the 56 warnings it reports are all real quality findings (orphan skills, unfinished content, stale python version, taint flow in merge-queue). happy to open a separate PR for those if you want, or leave them as advisory. they are not something that should block @rh-hemartin while i write this i see that the CI keeps failng. i think its because 6.1.1 is not publish yet. we should just wait a few minutes are re try |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review sweep: 6 findings on the harness-eval CI integration, focused on drift between the PR description/checklist and the shipped lint.yml step, plus a few gaps in the scan invocations themselves (dependency pin, exclude-flag asymmetry, coverage claims, and the scaffold AGENTS.md hard-fail gate). Details inline.
|
|
||
| - run: make lint-all | ||
|
|
||
| - name: Lint agent configurations (harness-eval) |
There was a problem hiding this comment.
[HIGH] PR description/checklist claims SARIF, PR-comment, and Action-based features the diff doesn't implement
The PR body claims "SARIF output enables inline annotations on the PR diff via GitHub Code Scanning," "Summary comment posted on each PR with scan results," "Uses the harness-eval GitHub Action with pinned checkout action SHA per repo convention," and checks off "[x] Paths filter limits runs to agent config changes only." None of this matches the current diff.
Tracing through this PR's history: the first iteration (commit 5658eff) added a separate .github/workflows/harness-eval.yml using uses: redhat-community-ai-tools/harness-eval/.github/actions/harness-eval@main with a real paths: filter (CLAUDE.md, AGENTS.md, skills/, commands/, .mcp.json, internal/scaffold/fullsend-repo/**) and security-events:write/pull-requests:write permissions — matching the description. After maintainer feedback to fold this into lint.yml as a plain step, the author rewrote it as a raw pip install plus two harness-eval lint --preset recommended --fail-on-error shell invocations, with no paths: filter, no --format sarif, no upload-sarif step, and no comment-posting step. The permissions: block at the top of this workflow is still only contents: read. The PR's own latest completed CI run (run 30446609422, job test, step "Lint agent configurations (harness-eval)") shows plain-text "Setup Assessment" output only — no SARIF artifact, no PR comment.
Suggestion: Rewrite the PR description and checklist to match the shipped implementation (plain CLI lint output gated by --fail-on-error, no annotations, no comment, no paths filter), or adopt the upstream composite Action (pinned to a full SHA) with sarif: true, comment: true, and the security-events:write/pull-requests:write permissions it needs.
There was a problem hiding this comment.
Rewrote the PR description to accurately reflect what's shipped: plain pip install + CLI lint, no SARIF, no PR comment, no paths filter. Also added a "What it does NOT do" section to be explicit.
|
|
||
| - name: Lint agent configurations (harness-eval) | ||
| run: | | ||
| pip install -q "harness-eval @ git+https://github.com/redhat-community-ai-tools/harness-eval.git@07b62ae" |
There was a problem hiding this comment.
[MEDIUM] harness-eval pinned to an abbreviated, untagged git SHA — a regression from an already-fixed PyPI pin
pip install -q "harness-eval @ git+https://github.com/redhat-community-ai-tools/harness-eval.git@07b62ae" pins to a 7-character abbreviated SHA. This was already raised and fixed once earlier in this PR: a reviewer asked "Could we pin it + renovate bot?" and the author replied "pinned to harness-eval==6.1.0, renovate should be able to track it now" — a proper PyPI version pin. The current diff has regressed away from that fix back to a raw git URL with a short SHA. The referenced commit (07b62aed4e86fbefe15a2846eae586fc02d57b18) sits one commit ahead of the latest tagged release v6.2.1 (6b9f9f84e5...) on harness-eval's main branch and is contained in no tag. Every uses: reference in every workflow file in this repo is pinned to a full 40-character SHA per the repo's checkout-pin convention; this pip-installed dependency follows neither that convention nor the PyPI-pin approach already agreed to earlier in this PR. Because it's inside a run: block rather than a uses: step, this repo's own SHA-pin checker (run via make lint-all in this same job) can't see or enforce it, and there's no obvious automated update path via Renovate for this pip+git URL pattern. It's also self-reported misleadingly: CI prints "Evaluated with: harness-eval v6.2.1 (cli)," indistinguishable from installing the real tagged v6.2.1 release, even though this is actually a newer, unreleased commit.
Suggestion: Pin to the full 40-character SHA with a comment explaining why a pre-release commit is used, and either add a Renovate customManagers entry for this pip+git pattern or switch back to a tagged harness-eval==X.Y.Z PyPI pin once those fixes are released, consistent with the fix already agreed to earlier in this PR.
There was a problem hiding this comment.
Pinned to full SHA 07b62aed4e86fbefe15a2846eae586fc02d57b18 with a comment explaining why it's a pre-release commit
| - name: Lint agent configurations (harness-eval) | ||
| run: | | ||
| pip install -q "harness-eval @ git+https://github.com/redhat-community-ai-tools/harness-eval.git@07b62ae" | ||
| harness-eval lint . --preset recommended --fail-on-error --exclude ".github/scripts/*" --exclude ".github/workflows/*" |
There was a problem hiding this comment.
[MEDIUM] "Advisory security gate" claim contradicted by the recommended preset's error-level security rules
Issue #5509 asks that "Security findings should also run but as advisory, not blocking, until the team is comfortable with signal quality," and the PR description asserts this is already true ("Security gate is advisory (non-blocking) until the team validates signal quality"). However, in harness-eval's recommended preset (as of the pinned commit 07b62ae), 8 security rules (security/no-prompt-injection, security/no-credential-access, security/reverse-shell, security/obfuscation, security/data-exfiltration, security/coercive-override, security/stealth-persistence, security/prompt-exfiltration, plus their agent/*/command/* equivalents) are marked "error", not "warning". Both invocations here run --preset recommended --fail-on-error, so any of those findings triggers an immediate failure — there's no advisory treatment for this rule family today; only the deeper heuristic checks (ast-behavioral, taint-flow, bash-taint-flow, mcp-least-privilege, mcp-tool-poisoning, cross-component-flow, memory-write-unscoped, unbounded-delegation) are "warning". Notably, harness-eval's own composite Action treats advisory security as a genuinely separate concept, exposing distinct security-gate (--fail-on-warning) and lint-gate (--fail-on-error) inputs precisely so security and structural findings can be gated independently. This PR's single lint --fail-on-error invocation conflates both.
Suggestion: Either drop the "advisory security" claim from the PR description (today, the 8 security/* pattern rules block immediately, same as structural rules), or implement the advisory intent — e.g. run harness-eval security . --fail-on-warning as a separate, genuinely non-blocking step matching the upstream Action's security-gate pattern, keeping lint --fail-on-error scoped to structural rules only.
There was a problem hiding this comment.
The job itself is advisory (not a required status check), so security findings won't block merge regardless of what the preset does internally. Splitting lint vs security into separate steps is a valid improvement, will track as a follow-up once we see what findings the recommended preset actually produces on this repo
| run: | | ||
| pip install -q "harness-eval @ git+https://github.com/redhat-community-ai-tools/harness-eval.git@07b62ae" | ||
| harness-eval lint . --preset recommended --fail-on-error --exclude ".github/scripts/*" --exclude ".github/workflows/*" | ||
| harness-eval lint internal/scaffold/fullsend-repo --preset recommended --fail-on-error |
There was a problem hiding this comment.
[MEDIUM] Root-scan --exclude flags are not mirrored on the scaffold invocation
The root invocation excludes .github/scripts/* and .github/workflows/* because those directories previously produced false positives during this PR's own review (a reviewer flagged security/no-credential-access in .github/scripts/install-openshell.sh and .github/workflows/functional-tests.yml, both legitimately requiring sudo), and harness-eval v6.2.1's --exclude flag was added specifically to suppress them. The scaffold invocation (harness-eval lint internal/scaffold/fullsend-repo --preset recommended --fail-on-error) has no --exclude at all, even though internal/scaffold/fullsend-repo/.github/ has the identical structure (a scripts dir plus 9 workflow files). Per this PR's own passing CI run, all of those files are scanned and downgraded to "Uncategorized Files (security-only rules applied)" — the same rule family that broke the root scan before the exclude was added there. It's currently 0 errors, but the asymmetry leaves the scaffold scan exposed to the same class of false positive the root scan already had to work around once.
Suggestion: Apply the same --exclude ".github/scripts/*" --exclude ".github/workflows/*" flags to the scaffold invocation for consistency, unless there's a specific reason the scaffold's copies of these files should be held to a stricter scan than the root's.
| run: | | ||
| pip install -q "harness-eval @ git+https://github.com/redhat-community-ai-tools/harness-eval.git@07b62ae" | ||
| harness-eval lint . --preset recommended --fail-on-error --exclude ".github/scripts/*" --exclude ".github/workflows/*" | ||
| harness-eval lint internal/scaffold/fullsend-repo --preset recommended --fail-on-error |
There was a problem hiding this comment.
[MEDIUM] PR description overstates actual scan coverage of the scaffold directory
The PR summary states the workflow "Scans both repo root... and scaffold (internal/scaffold/fullsend-repo/)," implying coverage comparable to the root scan. Without --recursive (a real harness-eval CLI flag), actual coverage is much narrower: this PR's own CI run shows the scaffold invocation finds only 1 real "component" (AGENTS.md) plus files swept under .github/ for security-only checks, finishing in 0.1s ("Components: 1", "11 components inspected, 0 errors, 1 warnings"). internal/scaffold/fullsend-repo/profiles/, providers/, scripts/, and templates/ are never inspected at all, despite plausibly qualifying as "agent configuration" under the same definition used to justify scanning the root.
Suggestion: Add --recursive to the scaffold invocation if full-tree coverage is the actual goal, or narrow the PR description to state precisely what's scanned today (AGENTS.md plus a security-only sweep of .github/).
There was a problem hiding this comment.
Fixed in the updated PR description
| run: | | ||
| pip install -q "harness-eval @ git+https://github.com/redhat-community-ai-tools/harness-eval.git@07b62ae" | ||
| harness-eval lint . --preset recommended --fail-on-error --exclude ".github/scripts/*" --exclude ".github/workflows/*" | ||
| harness-eval lint internal/scaffold/fullsend-repo --preset recommended --fail-on-error |
There was a problem hiding this comment.
[MEDIUM] Hard-fail gate targets scaffold AGENTS.md whose long-term repo ownership is unresolved
harness-eval lint internal/scaffold/fullsend-repo --preset recommended --fail-on-error is a hard-failure gate (test is a required status check per this repo's branch ruleset) that includes internal/scaffold/fullsend-repo/AGENTS.md. This repo's own docs/plans/agent-extraction-to-agents-repo.md ("Risks and considerations," item 6) states: "AGENTS.md in scaffold: The scaffold's AGENTS.md file contains shared agent rules that all agents reference. This file should be copied to the agents repo or made available via the harness layering mechanism" — i.e. its long-term source of truth is explicitly unresolved and trending toward fullsend-ai/agents (most other agent content has already been removed from this directory). By contrast, the .github/workflows/*.yml templates also scanned here are explicitly meant to stay in fullsend per the same doc's Risk item 4, so this concern is narrower than "the whole scaffold is contested" — it's specifically about AGENTS.md. Wiring a hard-fail gate onto a file whose home repo isn't settled means a future lint finding there could block unrelated fullsend PRs over content whose actual fix may belong in a different repo.
Suggestion: Until Risk item 6 is resolved, run the scaffold's AGENTS.md scan advisory-only (no --fail-on-error), or explicitly note in this PR/README which repo is authoritative for fixing scaffold-AGENTS.md findings, so a future contributor isn't blocked patching the wrong tree.
There was a problem hiding this comment.
Since the job is advisory (non-required), a future lint finding on AGENTS.md won't block unrelated PRs. Acknowledge the ownership concern, but the risk is mitigated by the job being non-blocking
8cd2646 to
fec8ece
Compare
|
Addressed the review findings in the latest push:
On the "advisory security gate" concern: since the job itself is non-required, security findings from the recommended preset won't block merge regardless. Splitting lint vs security into separate steps is a valid improvement, will track as a follow-up once we see what findings the preset actually produces on this repo. Same reasoning for the scaffold AGENTS.md ownership concern; the non-blocking job mitigates the risk. @waynesun09 |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review sweep: 5 findings on the harness-eval job as it stands after the latest push — a pre-release pin now superseded by a tagged release, a silent-skip risk in the scaffold scan caused by shared-shell errexit, a coverage gap on the scaffold target and its real content elsewhere, a Renovate tracking gap for the pin, and a missing timeout-minutes. Details inline.
| # ($ENVVAR paths) and prompt-exfiltration (negated instructions). | ||
| # Switch back to harness-eval==X.Y.Z once a release contains these fixes. | ||
| run: | | ||
| pip install -q "harness-eval @ git+https://github.com/redhat-community-ai-tools/harness-eval.git@07b62aed4e86fbefe15a2846eae586fc02d57b18" |
There was a problem hiding this comment.
[HIGH] Pinned pre-release harness-eval commit already superseded by tagged v6.3.0 PyPI release
The job installs harness-eval @ git+https://github.com/redhat-community-ai-tools/harness-eval.git@07b62aed4e86fbefe15a2846eae586fc02d57b18 with a comment saying "Switch back to harness-eval==X.Y.Z once a release contains these fixes." I independently verified this condition is now met: PyPI's JSON API shows harness-eval 6.3.0 is published (uploaded 2026-07-30T10:33:53Z) and is the current info.version; GitHub's compare API for 07b62aed4e86fbefe15a2846eae586fc02d57b18...v6.3.0 returns ahead_by: 6, behind_by: 0, confirming the pinned commit is a strict ancestor of v6.3.0 (i.e. v6.3.0 contains this pin's fixes plus 5 more commits, including two additional false-positive fixes). I also checked the 6.3.0 CHANGELOG: its only "Breaking" entry is google-genai/anthropic moving to optional [llm] extras for review/security --review/skill --rubric — explicitly not affecting lint or security (scan-only), which is all this workflow uses, so the flags used here (--preset, --fail-on-error, --exclude, all present since 6.2.1) are unaffected. This is not a hypothetical future condition; the tagged, PyPI-published release this pin's own comment is waiting for already exists as of this review.
Suggestion: Change line 162 to pip install -q "harness-eval==6.3.0" and delete the git+https URL and the three-line pre-release-pin comment on lines 158-160.
| # Switch back to harness-eval==X.Y.Z once a release contains these fixes. | ||
| run: | | ||
| pip install -q "harness-eval @ git+https://github.com/redhat-community-ai-tools/harness-eval.git@07b62aed4e86fbefe15a2846eae586fc02d57b18" | ||
| harness-eval lint . --preset recommended --fail-on-error --exclude ".github/scripts/*" --exclude ".github/workflows/*" |
There was a problem hiding this comment.
[HIGH] Scaffold scan is silently skipped whenever the root scan fails (bash errexit in shared run block)
Lines 162-164 are one multi-line run: | block, and no shell: override exists anywhere in this file, so GitHub Actions executes it with the default Linux shell bash --noprofile --norc -eo pipefail {0} (errexit on). Both harness-eval lint invocations use --fail-on-error, which exits non-zero the moment an error-severity finding appears. If line 163 (root scan) fails, bash aborts immediately and line 164 (the scaffold scan) never runs, with no log message indicating it was skipped. I reproduced both invocations locally against this PR's head commit (fec8ece): today both exit 0 (root: 27 components/646 rules/0 errors/56 warnings; scaffold: 1 component/0 rules/0 errors), so this is currently latent — but the recommended preset's security rules have documented recent false-positive history (this PR's own pin exists to fix broken-references false positives on bare $ENVVAR paths and prompt-exfiltration false positives on negated instructions), so an error-level false positive on the root scan is a realistic near-term trigger. This directly contradicts the PR's and issue #5509's stated goal that "both [root and scaffold] should be scanned."
Suggestion: Split into two separate - name: steps (one per target directory) so each runs and reports independently of the other's exit status, then aggregate: e.g. give the second step if: always() and add a final step that checks both step outcomes and fails the job if either failed. Alternatively, run both invocations with set +e, capture $? for each, restore set -e, and exit with the combined status.
| run: | | ||
| pip install -q "harness-eval @ git+https://github.com/redhat-community-ai-tools/harness-eval.git@07b62aed4e86fbefe15a2846eae586fc02d57b18" | ||
| harness-eval lint . --preset recommended --fail-on-error --exclude ".github/scripts/*" --exclude ".github/workflows/*" | ||
| harness-eval lint internal/scaffold/fullsend-repo --preset recommended --fail-on-error --exclude ".github/scripts/*" --exclude ".github/workflows/*" |
There was a problem hiding this comment.
[MEDIUM] Scaffold lint target applies zero rules today; the repo that holds the real content has no coverage
I ran the exact pinned command locally against this PR's head commit: harness-eval lint internal/scaffold/fullsend-repo --preset recommended --fail-on-error --exclude ".github/scripts/*" --exclude ".github/workflows/*" discovers exactly 1 component (AGENTS.md, 528 tokens) and reports "Duration: 0.0s | Components: 1 | Rules: 0" — literally zero lint rules are applied. Issue #5509 justified scanning this path because the scaffold "has harness files... (AGENTS.md, skills, schemas)", but per ADR-0058 ("Scaffold-embedded harnesses (formerly internal/scaffold/fullsend-repo/harness/...) now in fullsend-ai/agents") that content already moved out. I confirmed via the repo tree at this PR's head commit that internal/scaffold/fullsend-repo has no harness/, skills/, or schemas/ directories left — only empty customized/*/.gitkeep placeholders (ADR-0064 confirms "The customized/ directories currently contain only .gitkeep placeholders"). Meanwhile fullsend-ai/agents now hosts the real agents/*.md, CLAUDE.md, AGENTS.md, config.yaml content harness-eval is designed to catch problems in, and I confirmed its own .github/workflows/lint.yml runs only pre-commit + commit-lint — no harness-eval or equivalent step. A prior review round already flagged that the PR description overstated scaffold coverage (fixed by rewording the description) and that AGENTS.md's long-term ownership is unresolved (acknowledged as low-risk since the job is advisory); this finding is narrower and adds new verified information: regardless of wording or ownership, the scan target applies zero rules today, and the repo that actually holds the content harness-eval targets (skills, harness definitions, prompt-injection-prone agent instructions) has no static analysis at all.
Suggestion: Add the harness-eval lint step to fullsend-ai/agents' own CI, where the real agent/harness/skill content now lives and currently has zero static-analysis coverage. If a scaffold-directory check stays in fullsend-ai/fullsend, document that it only covers the residual AGENTS.md file rather than delivering the skills/schemas coverage issue #5509 originally asked for.
| # ($ENVVAR paths) and prompt-exfiltration (negated instructions). | ||
| # Switch back to harness-eval==X.Y.Z once a release contains these fixes. | ||
| run: | | ||
| pip install -q "harness-eval @ git+https://github.com/redhat-community-ai-tools/harness-eval.git@07b62aed4e86fbefe15a2846eae586fc02d57b18" |
There was a problem hiding this comment.
[MEDIUM] harness-eval pin has no Renovate update path in its current git+SHA form
renovate.json defines 5 customManagers regex entries (OpenShell version in .github/scripts/openshell-version.sh, gcloud CLI version in images/runner/Containerfile, wrangler version in .github/workflows/site-deploy.yml, Claude Code version and tirith version in images/sandbox/Containerfile) specifically because pins embedded inside scripts/Containerfiles/workflow run: blocks are invisible to Renovate's built-in managers. None of their managerFilePatterns match .github/workflows/lint.yml, and Renovate's default Python managers (pip-requirements, pip_setup, pep621, poetry) only parse dedicated dependency files, not inline pip install commands inside workflow YAML run: blocks. So this pin — whether it stays a git+SHA reference or moves to a PyPI version string — has no automated update mechanism today; it will only ever be bumped when a human notices. Earlier in this PR's review a maintainer asked "Could we pin it + renovate bot?" and the author replied "pinned to harness-eval==6.1.0, renovate should be able to track it now" — an assumption that was never actually checked against Renovate's manager behavior, and the pin has since moved even further from anything trackable (from a plain PyPI version string to the current git+https://...@<sha> URL).
Suggestion: Add a customManagers entry to renovate.json matching .github/workflows/lint.yml, following the same regex-capture pattern as the existing 5 entries, so future harness-eval releases actually generate a Renovate PR instead of relying on someone noticing manually.
| - name: Run svelte-check | ||
| run: npm run check | ||
|
|
||
| harness-eval: |
There was a problem hiding this comment.
[MEDIUM] New harness-eval job omits the repo's mandatory timeout-minutes
docs/contributing/ci-workflows.md's "Timeout policy" section states "Every non-reusable workflow job must set timeout-minutes" (GitHub Actions otherwise defaults to a 6-hour timeout), and AGENTS.md's table lists that doc as required reading for "Adding or modifying GitHub Actions workflows under .github/workflows/" — exactly what this PR does. I confirmed via the fetched file that no job in the current lint.yml, including the new harness-eval job added by this PR, sets timeout-minutes. This job does a network pip install from a third-party git host followed by a filesystem/security scan — a plausible candidate for hanging (network stall, pathological scan) that the documented policy exists to guard against.
Suggestion: Add timeout-minutes: 5 (or similar, based on the job's actual ~1s observed runtime plus headroom) to the harness-eval job. The other pre-existing jobs in this file also lack timeout-minutes (a pre-existing gap outside this PR's scope), but a new job is a reasonable place to start complying with the documented convention.
fec8ece to
7b1673b
Compare
|
Addressed all findings:
On scaffold target applying zero rules today (MEDIUM): acknowledged. The scaffold extraction moved most content to fullsend-ai/agents, leaving only AGENTS.md. The scan still catches structural issues on that file. Adding harness-eval On Renovate tracking (MEDIUM): pushing back for now. The pip install is inside a run: block, not a uses: reference, so Renovate's default managers won't pick it up. Adding a customManagers entry to renovate.json is a good idea but |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Follow-up review pass (2 findings, both new since the last round — no duplicates of prior comments).
| python-version: "3.12" | ||
|
|
||
| - name: Install harness-eval | ||
| run: pip install -q "harness-eval==6.3.0" |
There was a problem hiding this comment.
[MEDIUM] PR description's "Pin note" section still describes a git-SHA pre-release pin that no longer exists in the shipped diff
The PR description's "Pin note" section reads: "Pinned to a pre-release commit (07b62ae) that includes fixes for broken-references false positives on bare $ENVVAR paths and prompt-exfiltration false positives on negated instructions. Will switch to a tagged PyPI release once one is cut with these fixes." That text is still present verbatim in the current PR description. But the shipped workflow at this head commit (line 159) reads pip install -q "harness-eval==6.3.0" — a plain PyPI version pin, not a git+SHA pin at all; there's no git+https URL or pre-release-pin comment anywhere in the file. The author's own PR comment confirms the switch happened ("Switched from pre-release git pin to harness-eval==6.3.0... verified: PyPI published, contains our false-positive fixes"), but the description's Pin note was never updated to match, so it now describes a pinning mechanism and a "not yet cut" release that both no longer reflect reality.
Suggestion: Update the PR description's Pin note to state the final shipped mechanism, e.g. "Pinned to the tagged PyPI release harness-eval==6.3.0, which supersedes the pre-release fixes (07b62ae) originally targeted" — so a reviewer who reads only the description isn't misled about supply-chain provenance.
There was a problem hiding this comment.
Updated the PR description. Removed the pre-release pin note and replaced it with a "False-positive suppression" section describing the baseline approach and the 6.3.0 PyPI pin
| run: pip install -q "harness-eval==6.3.0" | ||
|
|
||
| - name: Lint repo root | ||
| run: harness-eval lint . --preset recommended --fail-on-error --exclude ".github/scripts/*" --exclude ".github/workflows/*" |
There was a problem hiding this comment.
[MEDIUM] The --exclude flags used for both scan targets suppress all rules (structural + security) for the excluded paths, not just the credential-access false positive they were added to fix
Both the root scan (this line) and the scaffold scan use --exclude ".github/scripts/*" --exclude ".github/workflows/*", added specifically to suppress a security/no-credential-access false positive on legitimate sudo usage in .github/scripts/install-openshell.sh and .github/workflows/functional-tests.yml. Per harness-eval's own source (src/harness_eval/cli/lint.py help text: "--exclude: Glob patterns for files/dirs to exclude from scanning (repeatable)") and its test suite (tests/test_exclude.py, e.g. test_directory_glob_excludes), --exclude removes matched files from setup.components entirely before any rule runs — it's a full discovery-level exclusion, not a per-finding suppression. That means every rule (structural: broken-references, missing-descriptions; every security rule besides the one false positive) is silently skipped for the entire .github/scripts/ and .github/workflows/ trees, including any future agent-relevant content added there (e.g. an embedded prompt in a new reusable workflow). harness-eval's lint.py also supports a separate --baseline PATH option ("Path to baseline JSON file. Suppress baselined findings.") that suppresses specific findings while still scanning the rest of the tree for everything else — a mechanism that already exists and would be more targeted.
Suggestion: Generate a baseline file capturing just the two no-credential-access false positives and use --baseline instead of the broad --exclude, so future genuinely agent-relevant content added under .github/scripts/ or .github/workflows/ isn't silently skipped from all lint/security coverage.
There was a problem hiding this comment.
Replaced both --exclude flags with a checked-in .harness-eval-baseline.json that suppresses only the two no-credential-access false positives. The baseline captures specific finding hashes, so all other rules (structural and security) still run against .github/scripts/ and .github/workflows/. The scaffold scan never had false positives, so it drops the --exclude flags entirely and runs unfiltered
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Review sweep: 1 finding on the checked-in .harness-eval-baseline.json — the PR description's characterization of what it suppresses doesn't match its actual contents.
| @@ -0,0 +1,40 @@ | |||
| { | |||
There was a problem hiding this comment.
[HIGH] PR description undercounts and mischaracterizes what the baseline suppresses
The PR description states the baseline suppresses "two known security/no-credential-access false positives ... on legitimate sudo usage in install-openshell.sh and functional-tests.yml." Verified directly by reading .harness-eval-baseline.json at head commit ca413ffd44f1beedd4885a57362babacf87157cc: it actually contains 7 entries across 4 files and 2 rule categories — security/no-credential-access on install-openshell.sh (1), install-podman.sh (4 separate sudo invocations: tar, ln -sf, tee, apparmor_parser), and functional-tests.yml (1), plus a 7th entry for rule content/broken-references on skills/analyze-transcript/SKILL.md, a structural/content rule with no stated "sudo false positive" rationale and not mentioned anywhere in the PR body, commit messages, or review thread.
I confirmed via a fresh harness-eval baseline run against the PR's actual merge tree that this broken-references entry is a real, reproducible finding (hash c1ea7fad24ce996c matches exactly), so it is not a hallucinated/stale entry — it is a genuine suppression that the PR narrative simply omits and never justifies. A future maintainer reading only the PR description would materially misjudge both the scope (2 vs 7 findings, 2 vs 4 files) and the nature (assumes it's all sudo/security related, when one entry silently baselines a structural content rule) of what CI is no longer checking.
Suggestion: Update the PR description's "False-positive suppression" section to accurately state the baseline suppresses 6 security/no-credential-access findings across 3 files (install-openshell.sh, install-podman.sh x4, functional-tests.yml) plus 1 unrelated content/broken-references finding in skills/analyze-transcript/SKILL.md, and add a one-line rationale for the broken-references entry (e.g. "false positive on the templated <base-dir>/analyze-transcript.py path pattern") in the PR body or as a comment in the baseline file, so a future maintainer regenerating or auditing the baseline understands why a non-security finding is suppressed.
There was a problem hiding this comment.
Fixed. Updated the PR description's "False-positive suppression" section to itemize all 7 baseline entries: 6 security/no-credential-access across 3 files (install-openshell.sh, install-podman.sh x4, functional-tests.yml) plus the content/broken-references on skills/analyze-transcript/SKILL.md, with a rationale explaining it's a false positive on the templated path pattern .transcripts/run-/...
|
@waynesun09 what do you think we should do about future harness-eval versions? more new rules |
|
Since the last round of reviews i've been doing quite a bit of work on harness-eval itself, so updating this PR to match. @rh-hemartin @waynesun09 @ralphbean What changed in this push:
Tested locally against the merged state (branch + current main): root scan passes with 0 errors / 65 warnings, scaffold scan passes with 0 errors / 1 warning. One open item left for a follow-up PR: Renovate tracking for the |
rh-hemartin
left a comment
rh-hemartin
left a comment
There was a problem hiding this comment.
Waiting for @waynesun09 review to merge.
|
DCO missing |
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Automated static-analysis sweep of the harness-eval baseline and workflow config. Three findings below, all independently reproduced against the PR head commit.
| "file": ".github/scripts/install-openshell.sh", | ||
| "message_hash": "560a2bc83b5930ff" | ||
| }, | ||
| { |
There was a problem hiding this comment.
[CRITICAL] Baseline waives findings for a skill path that does not exist in this repo
Verified by cloning the PR head commit (6e72ce5) and running the pinned harness-eval==7.9.2 directly: skills/author-fullsend-augmentations/SKILL.md does not exist anywhere in the tree (a full recursive git-tree listing of skills/ shows only analyze-transcript, cutting-releases, e2e-health, filing-issues, merge-queue, mint-enroll, renumber-adr, replay-session, spec-refine(-github), spec-start(-github), topissues, writing-adrs, writing-user-docs). Running harness-eval harness-lint . --preset recommended --fail-on-error WITHOUT the baseline produces exactly ONE error total (security/no-credential-access on .github/scripts/install-openshell.sh:5) and zero content/broken-references errors anywhere — so the 3 baseline entries for the nonexistent SKILL.md are not suppressing anything real in this repo; they are dead weight.
This directly contradicts the latest commit message ("bump harness-eval to 7.9.0": "Regenerate baseline ... 3x author-fullsend-augmentations broken-references on example paths in anti-pattern documentation"), which claims these are real, verified false positives in this repo. The baseline was evidently generated against a different checkout (most plausibly the fullsend-ai/agents repo or a pre-repo-split tree, where that skill content actually lives) rather than against this commit of fullsend-ai/fullsend. Since --fail-on-error is used, any genuine new finding that happens to collide with one of these stale entries would not be reliably caught, and this shows the regeneration process wasn't actually validated against this repo before being committed.
Suggestion: Regenerate .harness-eval-baseline.json by running harness-eval against a clean checkout of this exact commit only (not a working tree mixed with another repo's content), drop the 3 dead author-fullsend-augmentations entries since no such file/finding exists here, and re-verify that every remaining baseline entry corresponds to a diagnostic harness-eval actually emits against this repo.
| "findings": [ | ||
| { | ||
| "rule_id": "security/no-credential-access", | ||
| "file": ".github/scripts/install-openshell.sh", |
There was a problem hiding this comment.
[MEDIUM] Baseline suppression is brittle: the hashed message embeds a line number, so any unrelated edit above the flagged line silently reintroduces the "suppressed" finding
Verified against the pinned harness-eval==7.9.2 source (baseline.py): message_hash = sha256(diagnostic.message.encode()).hexdigest()[:16], and the no-credential-access rule's message template is "Contains dangerous command '{{match}}' at line {{line}}" — the line number is part of the hashed text. Reproduced this live: cloned the PR head commit, ran harness-eval harness-lint . --preset recommended --fail-on-error --baseline .harness-eval-baseline.json (exit 0, 0 errors, confirming the baseline currently suppresses the one real finding), then prepended a single unrelated comment line to the top of .github/scripts/install-openshell.sh (shifting the sudo call from line 5 to line 6) and re-ran the same command: it now exits 1 with Summary: 1 error ... security/no-credential-access in .github/scripts/install-openshell.sh:6, i.e. the baseline entry no longer matches and the "suppressed" finding reappears as new, even though nothing security-relevant changed.
Suggestion: Either don't rely on baseline.json for rule messages whose text embeds a line number (harness-eval ships inline suppression comments — <!-- evaluator-ignore: rule-id --> for markdown — but that mechanism doesn't apply to shell files, so for install-openshell.sh consider a rule-level or path-level allowlist instead of a message-hash baseline), or explicitly document that this baseline is expected to go stale on unrelated edits above line 5 of that script and needs periodic regeneration, so the advisory job "breaking" after an innocuous diff isn't confusing.
| python-version: "3.12" | ||
|
|
||
| - name: Install harness-eval | ||
| run: pip install -q "harness-eval==7.9.2" |
There was a problem hiding this comment.
[MEDIUM] harness-eval pip install has no hash pinning or transitive-dependency lock, inconsistent with this workflow's own supply-chain standard
pip install -q "harness-eval==7.9.2" pins only the top-level package version; transitive dependencies float unpinned with no hash verification, and -q hides the resolved dependency tree from CI logs. This is inconsistent with the adjacent test job in the same file, which SHA-pins every uses: action (e.g. actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1) and additionally sha256-checksums the lychee and pinact tarballs it downloads over curl (echo "<hash> /tmp/lychee.tar.gz" | sha256sum -c). A compromised or yanked transitive dependency of harness-eval would run with contents: read on every pull_request/push/merge_group event with no integrity check.
Suggestion: Pin with a hash-locked requirements file (pip install --require-hashes -r requirements-harness-eval.txt), or install via uvx --from harness-eval==7.9.2 harness-eval (this workflow's test job already uses uv), and drop -q so CI logs show the exact resolved package set.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Two additional findings from a static-analysis review pass (review-only, no action requested beyond what's noted inline).
| python-version: "3.12" | ||
|
|
||
| - name: Install harness-eval | ||
| run: pip install -q "harness-eval==7.10.0" |
There was a problem hiding this comment.
[MEDIUM] PR description states harness-eval==7.9.0 but the workflow pins 7.10.0
The PR description says "Pinned to the tagged PyPI release harness-eval==7.9.0 (97 rules across 11 categories)," but the workflow at this head commit actually installs pip install -q "harness-eval==7.10.0". Both are real, distinct tagged PyPI releases, so this reads as a stale description left over from an earlier revision (the version bump commits show 7.9.0 → 7.9.1 → 7.9.2 → 7.10.0, and the description text was never updated past the first of those). A reviewer or future auditor who checks "what version is pinned" from the PR description alone would consult the wrong changelog/release.
Suggestion: update the PR description to say harness-eval==7.10.0 and re-confirm the baseline was actually generated/verified against 7.10.0 rather than 7.9.0.
| - name: Run svelte-check | ||
| run: npm run check | ||
|
|
||
| harness-eval: |
There was a problem hiding this comment.
[MEDIUM] Dropping the GitHub Action delivery and path-filter scoping requested by issue #5509 isn't explained
Issue #5509 proposed shipping harness-eval "as a GitHub Action that can be added with a single workflow file" and running it "on every PR touching agent configurations" (implying scoped path filtering). This job instead uses a bare pip install + CLI invocation with no path filter, and the PR description's own "What it does NOT do" section lists both deviations ("No GitHub Action; uses plain pip install + CLI invocation" and "No paths filter (runs on every PR, not just agent config changes)") without explaining why.
The disclosure itself is good (and was added in response to an earlier review comment on this thread), but the description still doesn't say why the Action route and path filtering were dropped, so it's unclear whether this was a deliberate, reviewed tradeoff or a silent scope reduction from the linked issue.
Suggestion: add a sentence explaining the rationale (e.g. no official Action ships with a repo-standard SHA pin to depend on; the lint is fast/cheap enough that path-filtering isn't worth the added workflow complexity), so this reads as an intentional decision rather than an unexplained gap versus #5509.
|
updated PR description. I've addressed the two findings from the latest review rounds: 1. Version clarificationUpdated the PR description to reflect that this is now pinned to
2. Rationale for approachAdded explicit explanation in the PR description for why we're using plain Why not GitHub Action + path filter? The approach is simpler to iterate on during the advisory phase: bare |
|
@Benkapner there are three commits not correctly sign with DCO, I would suggest you squash them into a single one, which is easier to manage. @waynesun09 we need your review again. |
|
Reworked this job to address the recurring baseline-drift and over-suppression findings at the root instead of patching them per sweep. head is now The core change: this is no longer the 97-rule
What changed in the diff:
What the gate reports on this repo now: So the gate surfaces exactly one real issue (a genuine skill name/dir mismatch), that single true positive is baselined so the job is green today and flips red only on new drift, and there are no false positives to explain. still advisory, not a required check. |
Add an advisory harness-eval job to the CI workflow (not a required check) that lints agent configuration with harness-eval harness-gate. The gate runs only the six validated gating-tier rules (>=97% precision, zero corpus false positives) and never loads LLM extras, so there is no broad suppression list to maintain. Pinned to harness-eval==7.12.0. A regenerated baseline records one pre-existing true positive (skills/filing-issues/SKILL.md name mismatch) so the job is green today and flags only new drift; the scaffold is clean and needs no baseline. Closes fullsend-ai#5509 Signed-off-by: Benjamin Kapner <bkapner@redhat.com>
935b471 to
550331d
Compare
|
Closing this PR because the scanner and the configured targets are misaligned.
The downstream artifacts retained in this repository under Please reopen this work as an integration in |
|
🤖 Finished Retro · ✅ Success · Started 5:07 PM UTC · Completed 5:19 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $4.79 |
Retro: PR #5510 — harness-eval static analysis for agent configurationsOutcome: Closed without merge after 35 days of review (Jul 23 – Aug 27, 2026). What happenedExternal contributor Benkapner opened PR #5510 to add an advisory
The author iterated the external Agent involvementZero fullsend agent runs were dispatched. PR #5510 is a fork PR from an external contributor. The dispatch routing gate requires the PR author to be an authorized collaborator or bot, so all 41 Evidence for existing issues
No new proposalsAll identified improvement themes are already tracked by open issues. The primary actionable item — skipping retro on PRs with zero agent interaction (#5619) — would have prevented this retro run entirely, saving token cost with no loss of insight. |
3 participants
Summary
Adds an advisory
harness-evalCI job that lints agent configuration files using harness-eval static analysis.What it does
harness-evaljob to the existinglint.ymlworkflow. It is not part of the requiredtestjob, so findings do not block merge (advisory until the team trusts it).harness-eval harness-gateagainst both the repo root (with a baseline) and the scaffold (internal/scaffold/fullsend-repo, no baseline).harness-gateruns only the six validated gating-tier rules (>=97% precision on >=50 re-derived findings, zero corpus false positives) and never loads LLM extras. This is deliberately not the 97-rulerecommendedpreset: most of those rules are heuristic (judgments about prose) and advisory by design, and they were the source of the earlier false positives that a broad suppression list had to paper over.Pinned to the tagged PyPI release
harness-eval==7.12.0(this release also fixes security rules leaking onto generic text files such as CI workflows and shell scripts).Baseline
A checked-in
.harness-eval-baseline.jsonrecords known findings so the job runs clean on day one and flags only new drift. Against the gate it contains a single entry, a genuine pre-existing true positive:frontmatter/format-validonskills/filing-issues/SKILL.md(frontmatternameFiling GitHub Issuesdoes not match directoryfiling-issues).The scaffold scan is clean and runs without a baseline. There is no
--excludesuppression.What it does NOT do
harness-gateis pure static analysis, deterministic and offline)Related Issue
Closes #5509
Checklist