Skip to content

ci(#5509): add harness-eval static analysis for agent configurations #5510

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Benkapner wants to merge 1 commit into from
Closed

ci(#5509): add harness-eval static analysis for agent configurations #5510

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions .github/workflows/lint.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,3 +143,28 @@ jobs:

- name: Run svelte-check
run: npm run check

harness-eval:

@waynesun09 waynesun09 Jul 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

@waynesun09 waynesun09 Aug 17, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

# Lint agent configurations with harness-eval (advisory, not required for merge).
runs-on: ubuntu-24.04
timeout-minutes: 5
steps:
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1

- uses: actions/setup-python@ece7cb06caefa5fff74198d8649806c4678c61a1 # v6.3.0
with:
python-version: "3.12"

- name: Install harness-eval
run: pip install -q "harness-eval==7.12.0"

# harness-gate runs only the validated gating-tier rules (>=97% precision,
# zero corpus false positives) and never loads LLM extras. The root baseline
# suppresses one pre-existing true positive so the job runs clean today and
# flags only new drift; the scaffold is clean and needs no baseline.
- name: Gate repo root
run: harness-eval harness-gate . --baseline .harness-eval-baseline.json

- name: Gate scaffold
if: always()
run: harness-eval harness-gate internal/scaffold/fullsend-repo
10 changes: 10 additions & 0 deletions .harness-eval-baseline.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{

@waynesun09 waynesun09 Aug 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

@Benkapner Benkapner Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-/...

"version": "1.0",
"findings": [
{
"rule_id": "frontmatter/format-valid",
"file": "skills/filing-issues/SKILL.md",
"message_hash": "df3375eb05be64c3"
}
]
}
Loading