joshtronic/igor

Watch

fix(validation): validated must mean real CI + real test (not files-present) #317

Merged

joshtronic merged 2 commits from fix/validator-real-ci into master

2026-07-02 03:20:26 +00:00

igor commented

2026-07-02 03:11:27 +00:00

Collaborator

Why

The root cause of the "onboarded but actually broken" facade: the readiness checks are existence checks, not functionality checks. A repo passed with a deploy-only workflow and the failing npm stub test — which is how I wrongly called parsley "validated."

Changes (`lib/repo-checks.sh`)

check_test_signal — reject the npm default stub (echo "Error: no test specified" && exit 1). A test script must actually be a test.
check_ci_workflow — require a workflow that triggers on pull_request AND runs a build/test/lint step. A deploy-only workflow (push:master + rsync) no longer counts.

Dogfooded against the fleet

repo	result
porksicle, scenekids, joshing.you, igor.bot, vps-showdown, certifiedtradejobs	✅ still validate
sharktank	❌ now fails (stub test) — fixed by #22
parsley	❌ now fails (stub test + no PR CI) — fixed by #9

So "validated" now means what we say it means.

⚠️ Merge order

Merge the repo fixes first — parsley #9 and sharktank #22 — or those two will briefly fail the stricter validation and reopen onboarding tickets until their fixes land. certifiedtradejobs already passes (its only issue was the deploy branch, PR #9 on that repo).

Tradeoff: check_ci_workflow now fetches each workflow file (a few more API calls per validation; validation failures aren't cached, so a transient network miss just re-checks next tick).

bash -n + check-sync green.

## Why The root cause of the "onboarded but actually broken" facade: the readiness checks are **existence checks**, not functionality checks. A repo passed with a deploy-only workflow and the failing npm stub test — which is how I wrongly called parsley "validated." ## Changes (`lib/repo-checks.sh`) - **`check_test_signal`** — reject the npm default stub (`echo "Error: no test specified" && exit 1`). A `test` script must actually be a test. - **`check_ci_workflow`** — require a workflow that triggers on **`pull_request`** AND runs a **build/test/lint** step. A deploy-only workflow (push:master + rsync) no longer counts. ## Dogfooded against the fleet | repo | result | |---|---| | porksicle, scenekids, joshing.you, igor.bot, vps-showdown, certifiedtradejobs | ✅ still validate | | **sharktank** | ❌ now fails (stub test) — fixed by #22 | | **parsley** | ❌ now fails (stub test + no PR CI) — fixed by #9 | So "validated" now means what we say it means. ## ⚠️ Merge order **Merge the repo fixes first** — parsley **#9** and sharktank **#22** — or those two will briefly fail the stricter validation and reopen onboarding tickets until their fixes land. certifiedtradejobs already passes (its only issue was the deploy branch, PR #9 on that repo). Tradeoff: `check_ci_workflow` now fetches each workflow file (a few more API calls per validation; validation failures aren't cached, so a transient network miss just re-checks next tick). `bash -n` + `check-sync` green.

igor added 1 commit

2026-07-02 03:11:27 +00:00

fix(validation): "validated" must mean real CI + real test, not files-present

Lint / check-sync (push) Successful in 4s

Details

Lint / check-sync (pull_request) Successful in 4s

Details

f3f6be076c

The readiness checks were shallow existence checks, so a repo looked
validated when it wasn't (parsley: a deploy-only workflow AND the failing
npm stub test both passed the old checks). Harden the two that lied:

- check_test_signal: reject the npm default stub (`echo "Error: no test
  specified" && exit 1`) -- require a real test script, not just any.
- check_ci_workflow: require a workflow that runs ON pull_request AND runs a
  build/test/lint step -- a deploy-only workflow (push:master + rsync) no
  longer counts as CI.

Dogfooded against the fleet: the 6 genuinely-set-up repos still validate;
sharktank (stub test) and parsley (stub test + no PR CI) now correctly FAIL
-- which is the point. Tradeoff: check_ci_workflow now fetches each workflow
(a few more API calls; failures aren't cached, so a transient miss re-checks
next tick).

MERGE AFTER the repo fixes (parsley#9, sharktank#22) -- otherwise those two
briefly fail validation and reopen onboarding until their fixes land.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011KXPafSYJPY6XkWHHk1Lfs

igor requested review from joshtronic

2026-07-02 03:11:28 +00:00

igor commented

2026-07-02 03:14:51 +00:00

Author

Collaborator

🤖 Review — `REQUEST_CHANGES` (automated)

CI for f3f6be07: success

Tightens two readiness checks in lib/repo-checks.sh so "validated" means a real test + real PR-triggered CI, not just files present. The intent is sound and the change is in scope, but I can't confirm it's safe as-is.

Blocking:

Unverifiable new dependencies (check_ci_workflow). The rewrite drops forgejo_repo_dir_has_match in favor of forgejo_repo_list_dir and forgejo_repo_get_file, neither of which is defined or touched in this diff. If either helper doesn't exist (or has a different signature/exit semantics), the while loop reads nothing and check_ci_workflow silently returns 1 for every repo — turning the intended "stricter" check into "nothing ever validates." bash -n won't catch a missing function; only runtime does. Please confirm both helpers exist in the repo (and cite where), since I can't see them here.
No test coverage for a behavior change. This PR changes what passes validation (stub-test rejection, PR-CI requirement) but adds/adjusts zero tests. The contract requires tests for behavior changes. "Dogfooded against the fleet" is a claim I can't verify from the diff — it's exactly the kind of assertion a regression test should pin down. Add a test that exercises: (a) the npm stub is rejected, (b) a real test script passes, (c) a deploy-only workflow fails, (d) a pull_request + test/lint workflow passes.

Non-blocking observations:

grep -q 'pull_request' matches the string anywhere in the file — including comments, paths-ignore, or a job name — not just the on: trigger. Loose, but errs toward accepting, so low risk.
The build/test/lint regex includes bare build, which many deploy workflows also contain; the pull_request gate is what actually excludes deploy-only workflows. Fine as long as that gate holds, but worth a comment.
The ⚠️ Merge order note (parsley #9 / sharktank #22 must land first) is an operational dependency the human merging needs to honor — flagging so it isn't lost.

CI is green, but given the missing helpers can't be confirmed and there are no tests locking in the new behavior, I can't clear this for merge.
===BODY===

_{Independent review by the harness on claude-opus-4-8 (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.}

### 🤖 Review — `REQUEST_CHANGES` _(automated)_ CI for `f3f6be07`: **success** Tightens two readiness checks in `lib/repo-checks.sh` so "validated" means a real test + real PR-triggered CI, not just files present. The intent is sound and the change is in scope, but I can't confirm it's safe as-is. **Blocking:** - **Unverifiable new dependencies (`check_ci_workflow`).** The rewrite drops `forgejo_repo_dir_has_match` in favor of `forgejo_repo_list_dir` and `forgejo_repo_get_file`, neither of which is defined or touched in this diff. If either helper doesn't exist (or has a different signature/exit semantics), the `while` loop reads nothing and `check_ci_workflow` silently returns 1 for *every* repo — turning the intended "stricter" check into "nothing ever validates." `bash -n` won't catch a missing function; only runtime does. Please confirm both helpers exist in the repo (and cite where), since I can't see them here. - **No test coverage for a behavior change.** This PR changes what passes validation (stub-test rejection, PR-CI requirement) but adds/adjusts zero tests. The contract requires tests for behavior changes. "Dogfooded against the fleet" is a claim I can't verify from the diff — it's exactly the kind of assertion a regression test should pin down. Add a test that exercises: (a) the npm stub is rejected, (b) a real test script passes, (c) a deploy-only workflow fails, (d) a `pull_request` + test/lint workflow passes. **Non-blocking observations:** - `grep -q 'pull_request'` matches the string anywhere in the file — including comments, `paths-ignore`, or a job name — not just the `on:` trigger. Loose, but errs toward accepting, so low risk. - The build/test/lint regex includes bare `build`, which many deploy workflows also contain; the `pull_request` gate is what actually excludes deploy-only workflows. Fine as long as that gate holds, but worth a comment. - The `⚠️ Merge order` note (parsley #9 / sharktank #22 must land first) is an operational dependency the human merging needs to honor — flagging so it isn't lost. CI is green, but given the missing helpers can't be confirmed and there are no tests locking in the new behavior, I can't clear this for merge. ===BODY=== --- <sub>Independent review by the harness on `claude-opus-4-8` (effort: high). The human reviewer is requested once Igor has reviewed; a human still merges.</sub>

igor added spent time

2026-07-02 03:14:51 +00:00

37 seconds

igor self-assigned this

2026-07-02 03:14:52 +00:00

igor commented

2026-07-02 03:19:04 +00:00

Author

Collaborator

🔧 Rework — round 1 (automated)

Addressed the review on claude-opus-4-8 at effort high — 1 new commit(s).

### 🔧 Rework — round 1 _(automated)_ Addressed the review on `claude-opus-4-8` at **effort high** — 1 new commit(s).

igor removed their assignment

2026-07-02 03:19:14 +00:00

igor added spent time

2026-07-02 03:19:14 +00:00

2 minutes 18 seconds

igor added 1 commit

2026-07-02 03:19:14 +00:00

test: add unit tests for repo-checks CI and test signal validation

Lint / check-sync (push) Successful in 4s

Details

Lint / check-sync (pull_request) Successful in 4s

Details