HOMELAB-2682: feat(forgejo): full head/base sha in forgejo_get_pr #102

Merged

pi-landing merged 1 commit from plane/HOMELAB-2682-get-pr-sha into main 2026-11-05 22:28:42 +00:00

Conversation 2 Commits 1 Files changed 4 +187 -5

pi-landing commented 2026-11-05 21:57:48 +00:00

Collaborator

Copy link

Why

forgejo_get_pr returned no head sha at all. A merge session told to verify the
exact head could not do it through the tool: the census shows 7 characters, the
branch list 8, and this tool nothing. On 2026-11-05 one session reached for
curl with its own token to read head.sha — this PR closes that
improvisation path, so no session needs a credentialed fetch to verify a head.

What

• Head sha: / Base sha: lines near the top of the output, carrying the API's
  full 40-hex commit id. The API response already carried head.sha /
  base.sha, so this is purely presentational — no new request, no new field in
  the API layer.
• A short or absent sha is reported as a problem (NOT a full 40-hex commit id … / UNKNOWN …), never passed off as the exact head. Silence here is what
  sends a session to curl.
• Every pre-existing output field keeps its exact shape and order (other callers
  parse the text); details still carries the whole PR.
• Tool description + promptSnippet + docs/forgejo.md now say a merge
  session verifies the exact head through this tool.

Sample output (fixture PR):

Homelab/pi-extensions #79: HOMELAB-1679: verification closure
Head sha: c7a74c7d4a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d
Base sha: 1d2c3b4a5f60718293a4b5c6d7e8f90a1b2c3d4e
State: OPEN  |  plane/HOMELAB-1679-closure→main
Author: aaron  |  Created: …
Mergeable: true  |  Merged: false
URL: …  |  Diff: …

Tests

• New forgejo/tests/get-pr-sha.test.ts (5 tests): full 40-hex values, labelled
  and above the rest of the details, existing fields unchanged, short sha flagged,
  absent sha UNKNOWN (never undefined).
• The fake PR fixture now uses real 40-hex head/base shas and carries
  diff_url/patch_url, so the default fixture no longer models an invalid sha.

npm ci && npm run typecheck && npm test        → 27/27 files, 707/707 tests
npm run test:deployed                          → 27/27 files loaded+passed, 707/707, typebox 1.3.34

Not merged here: one normal review seat follows.

## Why `forgejo_get_pr` returned no head sha at all. A merge session told to verify the exact head could not do it through the tool: the census shows 7 characters, the branch list 8, and this tool nothing. On 2026-11-05 one session reached for `curl` **with its own token** to read `head.sha` — this PR closes that improvisation path, so no session needs a credentialed fetch to verify a head. ## What - `Head sha:` / `Base sha:` lines near the top of the output, carrying the API's **full 40-hex** commit id. The API response already carried `head.sha` / `base.sha`, so this is purely presentational — no new request, no new field in the API layer. - A short or absent sha is reported **as a problem** (`NOT a full 40-hex commit id …` / `UNKNOWN …`), never passed off as the exact head. Silence here is what sends a session to curl. - Every pre-existing output field keeps its exact shape and order (other callers parse the text); `details` still carries the whole PR. - Tool `description` + `promptSnippet` + `docs/forgejo.md` now say a merge session verifies the exact head through this tool. Sample output (fixture PR): ``` Homelab/pi-extensions #79: HOMELAB-1679: verification closure Head sha: c7a74c7d4a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d Base sha: 1d2c3b4a5f60718293a4b5c6d7e8f90a1b2c3d4e State: OPEN | plane/HOMELAB-1679-closure→main Author: aaron | Created: … Mergeable: true | Merged: false URL: … | Diff: … ``` ## Tests - New `forgejo/tests/get-pr-sha.test.ts` (5 tests): full 40-hex values, labelled and above the rest of the details, existing fields unchanged, short sha flagged, absent sha `UNKNOWN` (never `undefined`). - The fake PR fixture now uses real 40-hex head/base shas and carries `diff_url`/`patch_url`, so the default fixture no longer models an invalid sha. ``` npm ci && npm run typecheck && npm test → 27/27 files, 707/707 tests npm run test:deployed → 27/27 files loaded+passed, 707/707, typebox 1.3.34 ``` Not merged here: one normal review seat follows.

pi-landing added 1 commit 2026-11-05 21:57:50 +00:00

HOMELAB-2682: feat(forgejo): forgejo_get_pr returns the full head and base shas ... af31b2f5e3

A merge session told to verify "the exact head" could not do it through the
tool: the census prints 7 characters, the branch list 8, and forgejo_get_pr
printed no sha at all. On 2026-11-05 a session reached for curl WITH ITS OWN
TOKEN to read head.sha — that improvisation path is what this closes.

- Head sha: / Base sha: lines, near the top, carrying the API's full 40-hex
  commit id. The API response already carried them, so this is presentational.
- A short or absent sha is reported AS a problem (NOT a full 40-hex commit id /
  UNKNOWN), never dressed up as the exact head.
- Every pre-existing output field keeps its exact shape and order; details still
  carries the whole PR.
- Tool description + docs/forgejo.md say a merge session verifies the head here.
- tests/get-pr-sha.test.ts pins the contract; the fake PR fixture now uses real
  40-hex head/base shas and carries diff_url/patch_url.

ld-reviewer commented 2026-11-05 22:14:00 +00:00

Collaborator

Copy link

Verdict of record — ld-reviewer — head af31b2f5e3: APPROVE

model: deepseek-flash
session: 01a1adbd-114b-757a-b84d-278b581280e3

Blockers

None.

What I verified on this head

• Head/base sha is present and full. Diff adds Head sha: / Base sha: lines built from pr.head?.sha / pr.base?.sha, which the existing GET /repos/{o}/{r}/pulls/{n} payload already carries (PullRequest → PRRef.sha: string); no new request, no API-layer field. Live read of PR 102 confirms the payload answers head.sha = af31b2f5e37751579825a42a0b142c039e3b09bc (40 hex) and base.sha = 271a523f1912a0d027c38f5ad586ccda4ebaf5d5 (40 hex), so the tool will print the exact head verbatim.
• Existing output unchanged. The diff is additive only: title line, State:, Author:, Mergeable:, URL: … | Diff:, the description block, the unparseable-ref early return, and details (raw PR) are byte-identical to main. No caller parses by line position — grep across this repo, ~/repos/lab-director, spec/ and memory/ finds no positional reader of the forgejo_get_pr text; in-repo consumers (tests/merge-identity.test.ts) read details, which is untouched. The fixture gaining diff_url/patch_url fixes a fixture that was modelling Diff: undefined.
• A test pins the full sha. forgejo/tests/get-pr-sha.test.ts (5 tests) asserts the fixture values are 40-hex, that the rendered text contains the full value (a slice would fail), that no 7-char truncation is emitted, that the lines sit above State:/Author: and each is one line, that details.head.sha/details.base.sha still carry full values, that a short sha is flagged NOT a full 40-hex commit id, and that an absent sha is UNKNOWN rather than undefined.
• Docs and description. docs/forgejo.md tool table now states the full 40-hex Head sha: / Base sha: and that this is how a merge session verifies the exact head with no curl and no token of its own; description + promptSnippet agree; the natural-language router string at index.ts:1374 is unchanged, so routing still works.
• Both shapes green on this head (TESTING.md). npm ci && npm run typecheck && npm test → typecheck clean, 27/27 files, 707/707 tests. npm run test:deployed → [deployed-shape] ok: 27/27 files loaded and passed, 707/707 tests, pinned tree typebox 1.3.34. Reproduced, matches the PR body.
• CI green on the head. Run #306: all 15 jobs success (forgejo, forgejo-ci, pi-swarm-suite, plane, redact, …); run #307 python success. Remote branch tip is the reviewed sha.

Concerns

• C1 — deployment, not this diff: the deployed source drop at ~/.pi/agent/extensions/forgejo/index.ts still has no shaLine (no occurrence of it), so until the extension is re-synced a session still sees no sha and the improvisation path stays open. Whoever lands this should ensure the drop is refreshed before the docs claim is relied on.
• C2 — wording nit: the short/absent warning string is shared by both labels, so a short Base sha: reads "do not treat this as the exact head". Accurate for the head line, slightly off for the base line. Cosmetic; not worth a fix round.
• C3 — shaLine validates case-insensitively but prints the value verbatim; harmless for the forge's lowercase ids, noted only so the asymmetry is deliberate.

Nothing here blocks: the change is presentational, additive, pinned by a test, documented, and green in both shapes plus CI.

**Verdict of record — ld-reviewer — head af31b2f5e37751579825a42a0b142c039e3b09bc: APPROVE** model: deepseek-flash session: 01a1adbd-114b-757a-b84d-278b581280e3 ## Blockers None. ## What I verified on this head - **Head/base sha is present and full.** Diff adds `Head sha:` / `Base sha:` lines built from `pr.head?.sha` / `pr.base?.sha`, which the existing `GET /repos/{o}/{r}/pulls/{n}` payload already carries (`PullRequest` → `PRRef.sha: string`); no new request, no API-layer field. Live read of PR 102 confirms the payload answers `head.sha` = `af31b2f5e37751579825a42a0b142c039e3b09bc` (40 hex) and `base.sha` = `271a523f1912a0d027c38f5ad586ccda4ebaf5d5` (40 hex), so the tool will print the exact head verbatim. - **Existing output unchanged.** The diff is additive only: title line, `State:`, `Author:`, `Mergeable:`, `URL: … | Diff:`, the description block, the unparseable-ref early return, and `details` (raw PR) are byte-identical to `main`. No caller parses by line position — grep across this repo, `~/repos/lab-director`, `spec/` and `memory/` finds no positional reader of the `forgejo_get_pr` text; in-repo consumers (`tests/merge-identity.test.ts`) read `details`, which is untouched. The fixture gaining `diff_url`/`patch_url` fixes a fixture that was modelling `Diff: undefined`. - **A test pins the full sha.** `forgejo/tests/get-pr-sha.test.ts` (5 tests) asserts the fixture values are 40-hex, that the rendered text contains the full value (a slice would fail), that no 7-char truncation is emitted, that the lines sit above `State:`/`Author:` and each is one line, that `details.head.sha`/`details.base.sha` still carry full values, that a short sha is flagged `NOT a full 40-hex commit id`, and that an absent sha is `UNKNOWN` rather than `undefined`. - **Docs and description.** `docs/forgejo.md` tool table now states the full 40-hex `Head sha:` / `Base sha:` and that this is how a merge session verifies the exact head with no `curl` and no token of its own; `description` + `promptSnippet` agree; the natural-language router string at `index.ts:1374` is unchanged, so routing still works. - **Both shapes green on this head (TESTING.md).** `npm ci && npm run typecheck && npm test` → typecheck clean, 27/27 files, 707/707 tests. `npm run test:deployed` → `[deployed-shape] ok: 27/27 files loaded and passed, 707/707 tests`, pinned tree typebox 1.3.34. Reproduced, matches the PR body. - **CI green on the head.** Run #306: all 15 jobs success (`forgejo`, `forgejo-ci`, `pi-swarm-suite`, `plane`, `redact`, …); run #307 `python` success. Remote branch tip is the reviewed sha. ## Concerns - C1 — deployment, not this diff: the deployed source drop at `~/.pi/agent/extensions/forgejo/index.ts` still has no `shaLine` (no occurrence of it), so until the extension is re-synced a session still sees no sha and the improvisation path stays open. Whoever lands this should ensure the drop is refreshed before the docs claim is relied on. - C2 — wording nit: the short/absent warning string is shared by both labels, so a short `Base sha:` reads "do not treat this as the exact head". Accurate for the head line, slightly off for the base line. Cosmetic; not worth a fix round. - C3 — `shaLine` validates case-insensitively but prints the value verbatim; harmless for the forge's lowercase ids, noted only so the asymmetry is deliberate. Nothing here blocks: the change is presentational, additive, pinned by a test, documented, and green in both shapes plus CI.

ld-reviewer commented 2026-11-05 22:26:03 +00:00

Collaborator

Copy link

Verdict of record — ld-reviewer — head af31b2f5e3: APPROVE

model: deepseek-flash
session: 1fc90a52-16c2-4d47-9d28-2b8fb27f4c4f

Blockers

None.

What I verified on this head (independent seat 2 of 2, dangerous class)

• The shas are the API's own full commit ids. shaLine (index.ts:186-197) formats pr.head?.sha / pr.base?.sha; PRRef.sha is a required string in the type (types.ts:139-144) and getPR (api.ts:1314-1324) is an unchanged single GET repos/{o}/{r}/pulls/{n}. A live read of PR 102 returns head.sha = af31b2f5e37751579825a42a0b142c039e3b09bc and base.sha = 271a523f1912a0d027c38f5ad586ccda4ebaf5d5, both 40 hex, so the tool prints the exact head with no derivation of its own.
• Dangerous-class lens — presentation only. The whole diff is 4 files, +187/-5: docs/forgejo.md, forgejo/index.ts, forgejo/tests/fake.ts, forgejo/tests/get-pr-sha.test.ts. In index.ts the additions are one regex constant, one pure string-formatting function, two appended lines to the display array, and the tool's description / promptSnippet text. No new request, no new fetch, no header construction touched (headers() unchanged), and landing.ts / verdict.ts / api.ts are untouched — nothing on the merge, lock or credential paths moves.
• No pre-existing output field changed. The insertions sit above State:; the title line, State:, Author:, Mergeable:, URL: … | Diff:, the description block, the unparseable repo_ref early return and details (the raw PR) are byte-identical to main. I grepped the package for a positional reader of this tool's text and found none — the only in-repo consumer reads details (tests/merge-identity.test.ts:183). The fixture gaining diff_url/patch_url is test-side: with the old fixture that line rendered Diff: undefined.
• The test cannot pass on a truncated sha — proved by mutation, not by reading. I temporarily changed the head-time return in shaLine to interpolate value.slice(0, 7) and ran npx vitest run tests/get-pr-sha.test.ts: 3 of 5 tests failed (the toContain('Head sha: <full 40>') assertion and the ^Head sha: [0-9a-f]{40}$ multiline match). I then restored the file and confirmed the worktree is clean and at af31b2f5e37751579825a42a0b142c039e3b09bc (git status --porcelain empty).
• Both shapes green (TESTING.md). tsc --noEmit exit 0; vitest run 27/27 files, 707/707 tests; bash scripts/test-in-deployed-shape.sh → [deployed-shape] ok: 27/27 files loaded and passed, 707/707 tests on the pinned tree typebox 1.3.34. CI on this head: run #306 all 15 jobs success, run #307 success.
• No credential in the new test. Its token literal is a per-file fake (23 chars, compared by value against the live FORGEJO_API_KEY: not equal, never printed), matching the convention that every suite carries its own fake; traffic is a route-table mockFetch, so the suite makes no network call and reads no credential.

Concerns

• C1 — deployment, not this diff: the deployed source drop at ~/.pi/agent/extensions/forgejo/index.ts has zero occurrences of shaLine, so a session using the extension as deployed still sees no sha and the improvisation this PR closes stays open until that drop is re-synced. The landing step should confirm the drop is refreshed.
• C2 — wording: the short-sha warning is shared by both labels and is worded for the head line, so a short Base sha: line reads oddly. Cosmetic.
• C3 — shaLine tests the value case-insensitively but prints it verbatim, so an uppercase-hex answer would be flagged as short. Theoretical for this forge; noted so the asymmetry is deliberate.
• C4 — nit: forgejo/tests/formal-review-fake.ts:123 still models sha: "base000", the same invalid-sha modelling this PR cleans up in fake.ts. That suite does not render this tool's text, so nothing breaks; it is fixture tidiness.

Nothing here blocks: the change is additive and presentational, the sha is the API's own full value, a test pins it against truncation, docs agree, and both test shapes plus CI are green on the reviewed head.

**Verdict of record — ld-reviewer — head af31b2f5e37751579825a42a0b142c039e3b09bc: APPROVE** model: deepseek-flash session: 1fc90a52-16c2-4d47-9d28-2b8fb27f4c4f ## Blockers None. ## What I verified on this head (independent seat 2 of 2, dangerous class) - **The shas are the API's own full commit ids.** `shaLine` (index.ts:186-197) formats `pr.head?.sha` / `pr.base?.sha`; `PRRef.sha` is a required `string` in the type (types.ts:139-144) and `getPR` (api.ts:1314-1324) is an unchanged single `GET repos/{o}/{r}/pulls/{n}`. A live read of PR 102 returns `head.sha` = `af31b2f5e37751579825a42a0b142c039e3b09bc` and `base.sha` = `271a523f1912a0d027c38f5ad586ccda4ebaf5d5`, both 40 hex, so the tool prints the exact head with no derivation of its own. - **Dangerous-class lens — presentation only.** The whole diff is 4 files, +187/-5: `docs/forgejo.md`, `forgejo/index.ts`, `forgejo/tests/fake.ts`, `forgejo/tests/get-pr-sha.test.ts`. In `index.ts` the additions are one regex constant, one pure string-formatting function, two appended lines to the display array, and the tool's `description` / `promptSnippet` text. No new request, no new `fetch`, no header construction touched (`headers()` unchanged), and `landing.ts` / `verdict.ts` / `api.ts` are untouched — nothing on the merge, lock or credential paths moves. - **No pre-existing output field changed.** The insertions sit above `State:`; the title line, `State:`, `Author:`, `Mergeable:`, `URL: … | Diff:`, the description block, the unparseable `repo_ref` early return and `details` (the raw PR) are byte-identical to `main`. I grepped the package for a positional reader of this tool's text and found none — the only in-repo consumer reads `details` (`tests/merge-identity.test.ts:183`). The fixture gaining `diff_url`/`patch_url` is test-side: with the old fixture that line rendered `Diff: undefined`. - **The test cannot pass on a truncated sha — proved by mutation, not by reading.** I temporarily changed the head-time return in `shaLine` to interpolate `value.slice(0, 7)` and ran `npx vitest run tests/get-pr-sha.test.ts`: 3 of 5 tests failed (the `toContain('Head sha: <full 40>')` assertion and the `^Head sha: [0-9a-f]{40}$` multiline match). I then restored the file and confirmed the worktree is clean and at `af31b2f5e37751579825a42a0b142c039e3b09bc` (`git status --porcelain` empty). - **Both shapes green (TESTING.md).** `tsc --noEmit` exit 0; `vitest run` 27/27 files, 707/707 tests; `bash scripts/test-in-deployed-shape.sh` → `[deployed-shape] ok: 27/27 files loaded and passed, 707/707 tests` on the pinned tree typebox 1.3.34. CI on this head: run #306 all 15 jobs success, run #307 success. - **No credential in the new test.** Its token literal is a per-file fake (23 chars, compared by value against the live `FORGEJO_API_KEY`: not equal, never printed), matching the convention that every suite carries its own fake; traffic is a route-table `mockFetch`, so the suite makes no network call and reads no credential. ## Concerns - C1 — deployment, not this diff: the deployed source drop at `~/.pi/agent/extensions/forgejo/index.ts` has zero occurrences of `shaLine`, so a session using the extension as deployed still sees no sha and the improvisation this PR closes stays open until that drop is re-synced. The landing step should confirm the drop is refreshed. - C2 — wording: the short-sha warning is shared by both labels and is worded for the head line, so a short `Base sha:` line reads oddly. Cosmetic. - C3 — `shaLine` tests the value case-insensitively but prints it verbatim, so an uppercase-hex answer would be flagged as short. Theoretical for this forge; noted so the asymmetry is deliberate. - C4 — nit: `forgejo/tests/formal-review-fake.ts:123` still models `sha: "base000"`, the same invalid-sha modelling this PR cleans up in `fake.ts`. That suite does not render this tool's text, so nothing breaks; it is fixture tidiness. Nothing here blocks: the change is additive and presentational, the sha is the API's own full value, a test pins it against truncation, docs agree, and both test shapes plus CI are green on the reviewed head.

pi-landing merged commit e9b02a3ade into main 2026-11-05 22:28:42 +00:00

pi-landing referenced this pull request from a commit 2026-11-05 22:28:42 +00:00

Merge pull request 'HOMELAB-2682: feat(forgejo): full head/base sha in forgejo_get_pr' (#102) from plane/HOMELAB-2682-get-pr-sha into main

pi-landing referenced this pull request 2026-11-06 03:09:12 +00:00

HOMELAB-2682: fix(forgejo): 'No blockers' headings, refusal naming, real-approval corpus #105

pi-landing referenced this pull request 2026-11-06 03:14:26 +00:00

HOMELAB-2682: fix(forgejo): 'No blockers' headings, refusal naming, real-approval corpus #105

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

Homelab/pi-extensions!102

No description provided.