igor commented

Collaborator

The bug

#304s auto-scaffold _scaffold_put used **PUT** for every contents-API write. But Forgejos contents API uses POST to create a file and PUT to update one (PUT requires the blob sha). So creating CLAUDE.md or a lint config returned 422 [SHA]: Required — every scaffold that had to create a file bailed.

It only worked when the sole gap was a package.json test-script bump (an update, with sha) — which is exactly why certifiedtradejobs scaffolded (#7, package.json only) but parsley did not (it needed CLAUDE.md + lint created).

The fix

POST when theres no sha (create), PUT when there is (update). One-line branch on method`.

Verified live

Ran the fixed code against parsley end-to-end → opened joshtronic/parsleymakesitbetter.com#6 with CLAUDE.md and .markdownlint.json both created. So parsley is already unblocked; this PR is to get the fix into the live harness for every future repo.

Why it slipped

#304s unit tests cover the pure functions (gap parse, stack detect, content gen). This impure path (the actual API create) isnt unit-testable without hitting Forgejo — caught by verifying the live behavior, which is exactly what surfaced it.

Refs igor#304.

## The bug #304`s auto-scaffold `_scaffold_put` used **PUT** for every contents-API write. But Forgejo`s contents API uses **POST to create** a file and **PUT to update** one (PUT requires the blob `sha`). So creating `CLAUDE.md` or a lint config returned **422 `[SHA]: Required`** — every scaffold that had to *create* a file bailed. It only worked when the sole gap was a `package.json` test-script bump (an *update*, with sha) — which is exactly why **certifiedtradejobs scaffolded (#7, package.json only) but parsley did not** (it needed CLAUDE.md + lint *created*). ## The fix POST when there`s no sha (create), PUT when there is (update). One-line branch on `method`. ## Verified live Ran the fixed code against parsley end-to-end → opened **joshtronic/parsleymakesitbetter.com#6** with `CLAUDE.md` and `.markdownlint.json` both **created**. So parsley is already unblocked; this PR is to get the fix into the live harness for every future repo. ## Why it slipped #304`s unit tests cover the pure functions (gap parse, stack detect, content gen). This impure path (the actual API create) isn`t unit-testable without hitting Forgejo — caught by verifying the live behavior, which is exactly what surfaced it. Refs igor#304.

igor added 1 commit

2026-07-01 21:39:29 +00:00

fix(onboarding): scaffold must POST to create files, PUT only to update (igor#304)

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

Details

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

Details

ca8c2280b0

#304's _scaffold_put used PUT for every contents-API write. Forgejo's API
uses POST to create a file and PUT to update one (PUT requires the blob
sha), so creating CLAUDE.md or a lint config returned 422 "[SHA]:
Required" -- every scaffold that had to CREATE a file bailed. It only ever
worked when the sole gap was a package.json test-script bump (an update,
with sha), which is why certifiedtradejobs scaffolded but parsley didn't.

POST when there's no sha (create), PUT when there is (update). Verified
live: the fixed code scaffolded parsley (CLAUDE.md + .markdownlint.json
created) in joshtronic/parsleymakesitbetter.com#6.

The #304 unit tests cover the pure functions; this impure API path isn't
unit-testable without hitting Forgejo, so it slipped -- caught by verifying
the live behavior.

Refs igor#304.

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-01 21:39:30 +00:00

igor commented

2026-07-01 21:41:08 +00:00

Author

Collaborator

🤖 Review — `APPROVE` (automated)

CI for ca8c2280: success

One-line fix: _scaffold_put now POSTs to create a file (no sha) and PUTs to update (sha present), matching Forgejo's contents API — resolves the 422 [SHA]: Required on create.

lib/repo-checks.sh — correct: method=POST in the no-sha branch, method=PUT in the sha branch, and _fj "$method" ... replaces the hardcoded PUT. The two jq bodies already differed correctly (POST body omits sha, PUT includes it), so this is consistent.
Scope is tight (~15 lines, one function), CI is success, no CI-config or security surface touched.
No unit test added, but the PR is honest about why: this is the impure API-write path that can't be exercised without hitting Forgejo. The prior #304 tests cover the pure functions, and the author reports live end-to-end verification against parsley (parsleymakesitbetter.com#6). Reasonable given the constraint.

No blocking concerns.
===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 — `APPROVE` _(automated)_ CI for `ca8c2280`: **success** One-line fix: `_scaffold_put` now POSTs to create a file (no sha) and PUTs to update (sha present), matching Forgejo's contents API — resolves the 422 `[SHA]: Required` on create. - `lib/repo-checks.sh` — correct: `method=POST` in the no-sha branch, `method=PUT` in the sha branch, and `_fj "$method" ...` replaces the hardcoded `PUT`. The two `jq` bodies already differed correctly (POST body omits `sha`, PUT includes it), so this is consistent. - Scope is tight (~15 lines, one function), CI is `success`, no CI-config or security surface touched. - No unit test added, but the PR is honest about why: this is the impure API-write path that can't be exercised without hitting Forgejo. The prior #304 tests cover the pure functions, and the author reports live end-to-end verification against parsley (parsleymakesitbetter.com#6). Reasonable given the constraint. No blocking concerns. ===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.

igor added spent time

2026-07-01 21:41:08 +00:00

13 seconds

igor added 1 commit

2026-07-01 22:52:05 +00:00

fix(onboarding): scaffold PR requests the reviewer (unvalidated repos skip shadow review) (igor#304)

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

Details

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

Details

3e1ad9b4e1

The scaffold PR opens on an unvalidated repo, which the shadow-review tick
skips -- so nothing surfaces it to the human. Request FORGEJO_REVIEWER
directly at open. This is the one place a bot PR must request review,
precisely because there's no shadow pass to defer to.

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

igor commented

2026-07-01 23:02:23 +00:00

Author

Collaborator

🤖 Review — `REQUEST_CHANGES` (automated)

CI for 3e1ad9b4: success

The POST-vs-PUT fix itself is correct and well-reasoned — but the diff bundles a second, undisclosed behavioral change that the description never mentions.

The core fix (good): Branching on sha to choose POST (create) vs PUT (update) in _scaffold_put matches Forgejo's contents API exactly. The method local and _fj "$method" ... change are correct. This part is in scope and matches the title/description.

Blocking — undisclosed scope creep in scaffold_try_open_pr:

The diff reverses a deliberate design decision. The old code opened the scaffold PR UNASSIGNED with an explicit comment explaining why (unvalidated repo → shadow-review skips it → surfacing rides on the onboarding ticket). The new code now passes "${FORGEJO_REVIEWER:-}" to forgejo_open_pr to request a reviewer directly, and rewrites the comment to justify the opposite.
This is a meaningful policy change to how bot PRs get surfaced to the human, and it has nothing to do with the POST/PUT bug the PR claims to fix. The description is 100% about the API verb; it says zero about changing review-request behavior. A reader trusting the description would merge this without realizing the surfacing model changed. That's precisely the kind of silent scope expansion the human relies on this review to catch.

Can't verify from the diff:

forgejo_open_pr is not shown. I can't confirm it accepts a 6th positional reviewer argument, nor that passing an empty string (FORGEJO_REVIEWER unset) degrades gracefully rather than erroring or requesting an empty reviewer. If that function doesn't already support this parameter, this change is either a no-op or a break.
FORGEJO_REVIEWER — I can't confirm this is the canonical variable name used elsewhere for the reviewer.

What "fixed" looks like:

Either split the reviewer-request change into its own PR (it's a separate decision that deserves its own review), or, if it genuinely belongs here, update the PR description to disclose and justify it and confirm forgejo_open_pr supports the reviewer argument.

Tests: No tests added, which the description honestly explains (impure API path, not unit-testable without Forgejo). Acceptable for the POST/PUT change given CI is green — but the reviewer-request change is also untested and undescribed, compounding the concern.

CI is success, so nothing fails on the objective signal; the block is on scope + the undisclosed design reversal, not on red CI.

_{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 `3e1ad9b4`: **success** The POST-vs-PUT fix itself is correct and well-reasoned — but the diff bundles a second, undisclosed behavioral change that the description never mentions. **The core fix (good):** Branching on `sha` to choose `POST` (create) vs `PUT` (update) in `_scaffold_put` matches Forgejo's contents API exactly. The `method` local and `_fj "$method" ...` change are correct. This part is in scope and matches the title/description. **Blocking — undisclosed scope creep in `scaffold_try_open_pr`:** - The diff reverses a deliberate design decision. The old code opened the scaffold PR *UNASSIGNED* with an explicit comment explaining why (unvalidated repo → shadow-review skips it → surfacing rides on the onboarding ticket). The new code now passes `"${FORGEJO_REVIEWER:-}"` to `forgejo_open_pr` to request a reviewer directly, and rewrites the comment to justify the opposite. - This is a meaningful policy change to how bot PRs get surfaced to the human, and it has **nothing to do with the POST/PUT bug** the PR claims to fix. The description is 100% about the API verb; it says zero about changing review-request behavior. A reader trusting the description would merge this without realizing the surfacing model changed. That's precisely the kind of silent scope expansion the human relies on this review to catch. **Can't verify from the diff:** - `forgejo_open_pr` is not shown. I can't confirm it accepts a 6th positional `reviewer` argument, nor that passing an empty string (`FORGEJO_REVIEWER` unset) degrades gracefully rather than erroring or requesting an empty reviewer. If that function doesn't already support this parameter, this change is either a no-op or a break. - `FORGEJO_REVIEWER` — I can't confirm this is the canonical variable name used elsewhere for the reviewer. **What "fixed" looks like:** - Either split the reviewer-request change into its own PR (it's a separate decision that deserves its own review), or, if it genuinely belongs here, update the PR description to disclose and justify it and confirm `forgejo_open_pr` supports the reviewer argument. **Tests:** No tests added, which the description honestly explains (impure API path, not unit-testable without Forgejo). Acceptable for the POST/PUT change given CI is green — but the reviewer-request change is *also* untested and undescribed, compounding the concern. CI is `success`, so nothing fails on the objective signal; the block is on scope + the undisclosed design reversal, not on red CI. </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.

igor added spent time

2026-07-01 23:02:23 +00:00

26 seconds

igor self-assigned this

2026-07-01 23:02:24 +00:00

igor commented

2026-07-01 23:07:28 +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-01 23:07:55 +00:00

igor added spent time

2026-07-01 23:07:55 +00:00

3 minutes 12 seconds

igor added 1 commit

2026-07-01 23:07:57 +00:00

refactor: open scaffold PR unassigned instead of requesting reviewer

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

Details

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

Details

b6716cc440

igor commented

2026-07-01 23:10:16 +00:00

Author

Collaborator

🤖 Review — `APPROVE` (automated)

CI for b6716cc4: success

One-line fix: _scaffold_put now POSTs when there's no blob sha (create) and PUTs when there is (update), matching Forgejo's contents API semantics. Verdict: APPROVE.

Correctness: The branch is right — Forgejo's contents API requires sha for PUT (update) and rejects PUT-without-sha as 422 [SHA]: Required. The new method var is set to POST/PUT inside each existing sha branch and threaded through to _fj, so the create path drops the sha field (already handled by the existing else-branch body) and uses POST. Logic is internally consistent.
Scope: Tightly focused, ~10 lines, single function, no drive-by changes. Well within contract.
Tests: None added, but the change is on an impure path that hits Forgejo's API and isn't unit-testable without a live server — the author's rationale is honest and matches how #304 structured its tests (pure functions only). Live-verified against parsley (parsleymakesitbetter.com#6). CI is green.
Security / CI config: No secrets, no injection surface changed (inputs are already base64-encoded and jq-quoted), no workflow files touched.
Description honesty: No checklist to fabricate; the narrative matches the diff exactly.

Minor note (non-blocking): the commit message string is still hardcoded to `igor#304` for both create and update — harmless, just a cosmetic artifact of the original scaffold. Nothing to fix here.

_{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 — `APPROVE` _(automated)_ CI for `b6716cc4`: **success** One-line fix: `_scaffold_put` now POSTs when there's no blob sha (create) and PUTs when there is (update), matching Forgejo's contents API semantics. Verdict: APPROVE. - **Correctness:** The branch is right — Forgejo's contents API requires `sha` for PUT (update) and rejects PUT-without-sha as 422 `[SHA]: Required`. The new `method` var is set to `POST`/`PUT` inside each existing sha branch and threaded through to `_fj`, so the create path drops the sha field (already handled by the existing else-branch body) and uses POST. Logic is internally consistent. - **Scope:** Tightly focused, ~10 lines, single function, no drive-by changes. Well within contract. - **Tests:** None added, but the change is on an impure path that hits Forgejo's API and isn't unit-testable without a live server — the author's rationale is honest and matches how #304 structured its tests (pure functions only). Live-verified against parsley (parsleymakesitbetter.com#6). CI is green. - **Security / CI config:** No secrets, no injection surface changed (inputs are already base64-encoded and jq-quoted), no workflow files touched. - **Description honesty:** No checklist to fabricate; the narrative matches the diff exactly. Minor note (non-blocking): the commit message string is still hardcoded to `igor#304` for both create and update — harmless, just a cosmetic artifact of the original scaffold. Nothing to fix here. ==== --- 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.

igor added spent time

2026-07-01 23:10:16 +00:00

18 seconds