fix(#44): harden vouch enforcement

[jflowers]

Summary

Fix the four remaining defects from #44: invalid review severity guidance, insufficient Vouch Check comment permission, unsafe Vouch DB error handling, and an unnecessary stale-workflow permission.

Changes

• Replace the schema-invalid important-severity term with high-severity.
• Grant Vouch Check issues: write so it can post its explanatory closure comment.
• Fail the Vouch Check without closing a PR when the canonical Vouch DB cannot be read, including 404 responses.
• Remove the unused actions: write permission from the stale workflow.

Testing

• uvx pre-commit run --all-files
• make lint
• make check-bundle

Validation notes

• make script-test reaches the existing harness-jira-test.sh assertion failure even though the expected Jira variables are present in harness/triage.yaml; this PR does not modify that test or harness.
• The Vouch DB failure path calls core.setFailed() and returns before the PR-close API call.

---

Closes #44

[github-actions]

Functional tests are running

Authorization passed for this commit. See the Functional Tests workflow for results.

[qodo-code-review]

PR Summary by Qodo

Harden Vouch enforcement against database read failures

🐞 Bug fix ⚙️ Configuration changes 📝 Documentation 🕐 10-20 Minutes

AI Description

• Prevents Vouch database outages from closing legitimate pull requests.
• Grants comment permission while removing unused stale-workflow access.
• Standardizes breaking-change review guidance on high severity.

Diagram

graph TD
  A["PR opened"] --> B["Vouch Check"] --> C["Canonical Vouch DB"] --> D{"Read succeeds?"}
  D -->|No| E["Fail check"]
  D -->|Yes| F{"Author vouched?"} -->|Yes| G["Leave PR open"]
  F -->|No| H["Close and explain"]

Loading

High-Level Assessment

The approach is appropriate: database read failures are separated from confirmed non-vouched results, preventing accidental closures while visibly failing enforcement. Treating read errors as non-vouched was unsafe, while retries or a mirrored database would add complexity without eliminating the need for this failure mode. The permission changes also follow least-privilege principles.

Files changed (3) +4 / -3

Bug fix (1) +3 / -1

vouch-check.ymlFail safely when the canonical Vouch database is unavailable +3/-1

Fail safely when the canonical Vouch database is unavailable

• Adds issue write access so the workflow can post closure explanations. Database read errors now fail the check and return immediately instead of treating the contributor as non-vouched and closing the pull request.

.github/workflows/vouch-check.yml

Documentation (1) +1 / -1

COMMITS.mdUse the supported high-severity review term +1/-1

Use the supported high-severity review term

• Replaces the invalid 'important-severity' wording with 'high-severity' in breaking-change review guidance.

COMMITS.md

Other (1) +0 / -1

stale.ymlRemove unused Actions write permission +0/-1

Remove unused Actions write permission

• Drops the stale workflow's unnecessary 'actions: write' permission while retaining access required to label, close, and delete branches.

.github/workflows/stale.yml

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Database outages remain untested ✓ Resolved 📜 Skill insight ▣ Testability

Description

The getContent catch path now calls core.setFailed and returns instead of continuing
enforcement, but the PR adds or updates no test for that branch. When the canonical Vouch database
is unavailable, the intended no-close behavior and failed job result therefore have no regression
constraint.

Code

.github/workflows/vouch-check.yml[R71-72]

+              core.setFailed(`Could not read VOUCHED.td: ${e.message}`);
+              return;

Relevance

●● Moderate

Behavioral test coverage requests are reasonable, but no close repository precedent establishes
mandatory tests for workflow catch paths.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The cited workflow lines introduce a new failure outcome and early return, while the diff contains
no new or modified test file exercising that behavior.

.github/workflows/vouch-check.yml[69-72]
Skill: code-implementation

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The workflow behavior changes when the canonical Vouch database cannot be read, but no corresponding test verifies that the job fails and the pull request remains open.

## Fix Focus Areas
- .github/workflows/vouch-check.yml[69-72]

## Recommended Fix
Add a workflow or script-level test that makes the content lookup fail, then assert that failure is reported and neither the pull-request update nor explanatory comment operation runs.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. API errors can corrupt workflow logs ✓ Resolved 📜 Skill insight ⛨ Security

Description

core.setFailed interpolates e.message into an error annotation without individually removing
::, ANSI escapes, or other control characters. A failed canonical database read supplies that
exception text to the GitHub Actions command channel, so unexpected API error content reaches
workflow log annotations.

Code

.github/workflows/vouch-check.yml[71]

+              core.setFailed(`Could not read VOUCHED.td: ${e.message}`);

Relevance

●● Moderate

Sanitization findings have mixed outcomes; accepted for workflow commands but rejected for
comparable interpolated values.

PR-#90
PR-#776
PR-#148

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The added failure annotation interpolates the API exception message without an explicit complete
sanitizer, while both cited rules require every workflow-command value to be sanitized individually
for all specified command-channel hazards.

.github/workflows/vouch-check.yml[69-72]
Skill: code-review
Skill: pr-review

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The canonical Vouch database failure path passes exception text into a GitHub Actions annotation without applying the required complete workflow-command sanitization.

## Fix Focus Areas
- .github/workflows/vouch-check.yml[71-71]

## Recommended Fix
Sanitize `e.message` before passing it to `core.setFailed`, neutralizing percent-encoded newlines, literal carriage returns and newlines, `::` sequences, ANSI escapes, and remaining control characters.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Workflow changes need human approval ✓ Resolved 📜 Skill insight § Compliance

Description

The PR changes permissions in stale.yml and expands permissions while altering enforcement logic
in vouch-check.yml, both under the protected .github/ path. Issue #44 explains the work, but
protected workflow modifications still require human review and must not be auto-approved.

Code

.github/workflows/vouch-check.yml[9]

+  issues: write

Relevance

● Weak

Recent precedent rejected protected-workflow human-approval annotation requests; governance is
process, not a code fix.

PR-#754

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The diff modifies two files under .github/, including an added write permission; the cited rule
mandates a protected-path finding even when the linked issue supplies justification.

.github/workflows/vouch-check.yml[7-10]
.github/workflows/stale.yml[8-11]
Skill: pr-review

---

Context sources

✅ Compliance rules (platform): 58 rules

✅ Skills: 4 invoked

  code-review

  code-implementation

  pr-review

  docs-review

✅ Cross-repo context — repo relationships

  Explored: repo: fullsend-ai/autonomy-analysis (sha: 46717359)

Review mode: ⚖️ Balanced: This modifies GitHub Actions permissions and failure behavior in a security-sensitive workflow, so a careful full review is warranted.

Tip of the day

💡 Did you know, you can keep summaries lean with Finding overflow, which tucks the rest behind 'View more'

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[waynesun09]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:02 PM UTC · Completed 10:16 PM UTC

Commit: 542ad1b · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $2.23

[fullsend-ai-review]

Review

Findings

Medium

• [protected-path] .github/scripts/vouch-check-test.sh, .github/workflows/stale.yml, .github/workflows/vouch-check.yml — This PR modifies files under the protected path .github/. The PR links to issue Fix 4 unresolved bugs from PR #32: COMMITS.md, vouch-check.yml, stale.yml #44 and its description explains the rationale for each change (permission grant + fail-closed hardening in vouch-check.yml, permission removal in stale.yml, new regression tests in vouch-check-test.sh), so sufficient context exists for the change. Human approval is always required for protected-path changes regardless of context.

Low

• [scope-creep] .github/workflows/vouch-check.yml:53 — The collaborator-permission-check catch block's non-404 branch is converted from log-and-continue into core.setFailed() + return. This is the getCollaboratorPermissionLevel() path, distinct from the VOUCHED.td retrieval path that issue Fix 4 unresolved bugs from PR #32: COMMITS.md, vouch-check.yml, stale.yml #44's bug fix(#1): update harness dest paths and restructure for forge #3 and the author's clarifying comment address ("the Vouch DB error path"). The 404 branch (the normal "not a collaborator" outcome) is unchanged, so this doesn't reintroduce a silent-close problem — it applies the same fail-closed pattern to a second, related code path not explicitly enumerated in the issue. Not observed to be resolved or newly authorized since the prior review; severity preserved. See also: [injection-pattern] finding at this location.
• [injection-pattern] .github/workflows/vouch-check.yml:53 — Of the three core.setFailed/core.warning call sites added or changed in this PR, two (VOUCHED.td catch, comment-post catch) route only sanitizeWorkflowValue(e.message) into the workflow-command channel. The collaborator-permission-check catch additionally interpolates e.status raw, without passing it through sanitizeWorkflowValue. e.status is an Octokit numeric HTTP status (or undefined), so it cannot itself carry ::, ANSI escapes, or newline sequences — practical exploitability is low — but it is the one interpolated operand in this diff not run through the sanitizer, so exhaustive per-input coverage is not demonstrated at this call site. See also: [scope-creep] finding at this location.
• [scope-creep] .github/workflows/vouch-check.yml:25 — The new sanitizeWorkflowValue() helper (stripping ANSI escapes, control characters, and :: sequences) is not one of the four bugs enumerated in issue Fix 4 unresolved bugs from PR #32: COMMITS.md, vouch-check.yml, stale.yml #44 or the author's clarifying comment. It is tightly coupled to the authorized change, though: the fix newly routes previously-console.log-only API error text into core.setFailed()/core.warning(), which is a workflow-command channel that didn't receive this text before — the sanitizer is defense-in-depth for a surface this same PR introduces, not an unrelated feature addition.
• [sub-agent-failure] N/A — The adversarial challenger pass returned an empty adjudicated_findings array for a non-empty (3-finding) input. Per orchestrator policy, an empty result after a non-empty input is treated as a likely parsing/context-truncation failure rather than a legitimate unanimous dismissal, so the pre-challenger finding set above was retained rather than cleared.

Previous run

Review

Findings

Medium

• [protected-path] .github/workflows/stale.yml, .github/workflows/vouch-check.yml — This PR modifies files under the protected path .github/. The PR links to issue Fix 4 unresolved bugs from PR #32: COMMITS.md, vouch-check.yml, stale.yml #44 and its description explains the rationale for each change (permission removal in stale.yml; permission addition and error-handling hardening in vouch-check.yml), so sufficient context exists for the change. Human approval is always required for protected-path changes regardless of context.

Low

• [scope-creep] .github/workflows/vouch-check.yml:42 — The PR also converts the collaborator-permission-check catch block's non-404 branch from log-and-continue into core.setFailed() + return. This is a second, distinct control-flow change beyond the four bugs enumerated in issue Fix 4 unresolved bugs from PR #32: COMMITS.md, vouch-check.yml, stale.yml #44 and beyond the author's clarifying comment, which addresses only the VOUCHED.td retrieval path. The 404 case (the normal "not a collaborator" outcome) is unchanged. It is a small, same-pattern fail-closed change in the same script rather than a new capability, so it does not block, but it is not explicitly authorized by the linked issue.

[jflowers]

Current script tests are failing due to this: #1165 issue

[ggallen]

/ok-to-test

[waynesun09]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:00 PM UTC · Completed 11:15 PM UTC

Commit: 6df6b85 · View workflow run →

Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $2.94

[fullsend-ai-review]

Risk Assessment: moderate (2/5)

Details

Tier 1 is pulled up by 3 protected .github/ paths and a direct CI workflow change but offset by zero security-sensitive files, no dependency changes, and a non-first-time non-bot author; Tier 2 shows low-to-moderate churn/authorship aside from the frequently-changed Makefile; Tier 3 is low given the PR is scope-proportionate to issue #44s 4 enumerated bugs with no unresolved discussion; the weighted composite rounds to a moderate risk score of 2.

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 12:28 PM UTC · Completed 12:36 PM UTC

Commit: 6df6b85 · View workflow run →

Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $1.02

[fullsend-ai-retro]

PR #1377 (fullsend-ai/agents) fixed the 4 bugs enumerated in issue #44 (itself filed by a prior retro on PR #32) and merged cleanly after human approval. The main finding: the fullsend-ai-review agent's first pass (run 35399513333, reviewing commit 542ad1b, a 10-line diff touching .github/workflows/vouch-check.yml) missed an unsanitized e.message interpolated into core.setFailed() (a GitHub Actions workflow-command-injection pattern) and a missing regression test for the new fail-closed behavior. Both were independently caught within the same review window by a third-party bot (Qodo, inline at PR creation) and by human reviewer ralphbean 3 days later, who explicitly asked the author to add sanitization and a test. Root cause, verified against the actual skill source at the commit the review run used: skills/pr-review/SKILL.md step 3e assigns scope_constraint (a hard cap overriding sub-agent judgment) purely by line count/mechanicalness, with no path-sensitivity — unlike step 3c-1's large-PR triage, which explicitly treats .github/** as security-critical and forces deep review via a 'path-pattern override,' but that mechanism is skipped entirely for small-PR mode. So a small diff to a privileged, permission-holding GitHub Actions workflow got the same 15-tool-call cap as a small docs typo fix, throttling the security sub-agent below the depth its own embedded instructions call for. Corroborating: the second review run (full scope, after the diff grew past the small-PR threshold) used the identical security.md rules to correctly catch a residual, more subtle sanitization gap (e.status not sanitized) — same sub-agent, same instructions, different outcome, purely due to scope tier. One proposal below targets this gap. Two other observations found supporting evidence but no new issue: the second review run logged a 'sub-agent-failure' fallback where the adversarial challenger returned an empty result for a non-empty (3-finding) input and the orchestrator conservatively retained the pre-challenger findings — this is the exact mechanism already tracked in open issue #675 (challenger empty-result fallback), no new evidence beyond confirming it fires in practice. Separately, the author's comment about intermittent make test failures in harness-jira-test.sh matches already-open issue #1165, which has a fix in flight (PR #1164); no action needed.

Proposals filed

• Small-PR scope_constraint (step 3e) has no path-pattern override for governance/security-critical paths (in fullsend-ai/agents)