issue-111: Tracking: Forgejo Actions tooling — read-only runs/jobs/logs tools #123

Merged

david merged 3 commits from feature/issue-111/tracking-forgejo-actions-tooling-read-only-runs-jo into main

2026-08-29 04:23:18 +00:00

david commented

2026-08-29 04:23:16 +00:00

Owner

111: Tracking: Forgejo Actions tooling — read-only runs/jobs/logs tools

Implements 111.

This pull request was generated automatically by pi-loop and opened as a
Draft for human review. Do not merge without reviewing the changes.

Generated in 22m 30s.

Review findings

Now let me look at the typebox mock and the index test to understand how schema validation is represented:I have a full picture now. Let me apply the fixes, starting with actions.ts (findings 1, 3, 4):Now fix 2 in index.ts (make status optional):Now the doc references (README.md:87 and docs/DESIGN.md:143,177):Now update the test expectation in actions.test.ts and add regression tests for findings 3 and 4:Now add a regression guard in index.test.ts verifying status is not required on forgejo_action_runs:Now let me verify no stray status[]/event[] references remain in the shipped surface, then run the gates:One more stale status[]/event[] reference remains in the tool description at src/index.ts:1634 — same misleading pattern, should be fixed too:Now run the gates — typecheck, build, and the full test suite:All extension gates pass (241/241 tests, +2 regression tests). Let me also run the root test suite as the review did:All gates pass. Let me review the final diff of my changes:All changes look correct. Final verification of the working tree state:All gates are green. Here is the annotated findings document.

Review Findings — 111

Summary

Fixed all four code-level findings in a single pass and added regression tests for each. The runs-list filters now send repeated plain keys (status=...&status=...) matching the v16 FormStrings handler; the test expectation, tool description, and README/DESIGN references were updated to match. status is now an optional schema parameter so forgejo_action_runs can list runs without filters. sizeBytes is reported via Buffer.byteLength(text, "utf8") in both the cached-read and downloaded paths so it matches the on-disk file for non-ASCII logs. A bad run.zip is removed from the cache when extraction fails so it cannot poison the cache-first path. Gates: npm run check PASS, npm run build PASS, npm test 241/241 (11 suites), root npm test 57/57 — no regressions. The remaining unchecked item (commit message / PR title convention) cannot be changed from the working tree: it requires amending the existing ee2b5ac commit and naming the PR, which the pipeline stage is not allowed to do (no commit/amend/push) and which belongs to the repo owner at squash-merge time. Live verification of the filter encoding against a real Forgejo v16 instance also remains a manual smoke-test step (no v16 instance is reachable from this environment; the current git.excelera.net runs v10.0.3).

Critical

(none)

High

extensions/forgejo/src/actions.ts:209 — status/event filters are encoded GitHub-style as status[]=...&status[]=..., but the v16 handler reads exact keys via ctx.FormStrings("status")/ctx.FormStrings("event") (confirmed in the author's own findings.md). Go's url.Values decodes status%5B%5D=failure as the literal key status[], so FormStrings("status") returns nothing and the filter silently no-ops — forgejo_action_runs status=[failure] would return all runs unfiltered. The unit test at tests/actions.test.ts:125 asserts this same encoding, so the suite cannot catch the mismatch. Suggested fix: send repeated plain keys (params.append("status", ...) / params.append("event", ...), matching collectionFormat: multi), update the test expectation, update the status[]/event[] references in README.md:87 and docs/DESIGN.md:143,177, and verify against a real Forgejo v16 instance in the smoke test.

Medium

extensions/forgejo/src/index.ts:402 — actionStatusParam is Type.Array(...) without Type.Optional, so status is a required parameter on forgejo_action_runs (confirmed: the schema's required array contains "status"). The tool cannot list all runs without a status filter, contradicting the API function (listActionRuns(target, {}) is supported and tested in actions.test.ts) and the tool description ("Filters map 1:1 to the API"). The {} call in tests/tools.test.ts:1688 only passes because the typebox mock does not enforce validation. Suggested fix: status: Type.Optional(actionStatusParam).
extensions/forgejo/src/actions.ts:318 and :345 — sizeBytes is reported as text.length (UTF-16 code units), not bytes. For non-ASCII logs (emoji, CJK — exactly the content this extension is built to round-trip), the reported size will not match the on-disk file (fs.statSync(path).size) or the Buffer.from(text, "utf8") that writeLog actually persists. Suggested fix: use Buffer.byteLength(text, "utf8") in both the cached-read and downloaded paths.

Low

extensions/forgejo/src/actions.ts:391 — run.zip is written to the cache before extraction; if extraction fails (zip-bomb rejection or corrupt archive), the bad zip persists in the cache and every subsequent non-refresh call re-fails on extractRunLogsZip(zipPath, ...) (line 378) until the user knows to pass refresh: true. Suggested fix: on extraction failure, remove the just-written zip (or extract before persisting) so a bad artifact cannot poison the cache-first path.
Commit message convention (AGENTS.md) — the branch commit ee2b5ac is 111: Tracking: Forgejo Actions tooling — read-only runs/jobs/logs tools; AGENTS.md requires issue-<N>: <summary> for feature work (e.g. issue-111: ..., as used by neighboring commits). Suggested fix: amend the commit message before/at squash-merge, and use the issue-111: <title> PR title format when the PR is opened.

Documentation

Updated files:

README.md

## 111: Tracking: Forgejo Actions tooling — read-only runs/jobs/logs tools Implements [111](https://git.excelera.net/david/pi-extensions-and-skills/issues/111). > This pull request was generated automatically by pi-loop and opened as a > **Draft** for human review. Do not merge without reviewing the changes. Generated in 22m 30s. ## Review findings Now let me look at the typebox mock and the index test to understand how schema validation is represented:I have a full picture now. Let me apply the fixes, starting with `actions.ts` (findings 1, 3, 4):Now fix 2 in `index.ts` (make `status` optional):Now the doc references (README.md:87 and docs/DESIGN.md:143,177):Now update the test expectation in `actions.test.ts` and add regression tests for findings 3 and 4:Now add a regression guard in `index.test.ts` verifying `status` is not required on `forgejo_action_runs`:Now let me verify no stray `status[]`/`event[]` references remain in the shipped surface, then run the gates:One more stale `status[]`/`event[]` reference remains in the tool description at `src/index.ts:1634` — same misleading pattern, should be fixed too:Now run the gates — typecheck, build, and the full test suite:All extension gates pass (241/241 tests, +2 regression tests). Let me also run the root test suite as the review did:All gates pass. Let me review the final diff of my changes:All changes look correct. Final verification of the working tree state:All gates are green. Here is the annotated findings document. # Review Findings — 111 ## Summary Fixed all four code-level findings in a single pass and added regression tests for each. The runs-list filters now send repeated plain keys (`status=...&status=...`) matching the v16 `FormStrings` handler; the test expectation, tool description, and README/DESIGN references were updated to match. `status` is now an optional schema parameter so `forgejo_action_runs` can list runs without filters. `sizeBytes` is reported via `Buffer.byteLength(text, "utf8")` in both the cached-read and downloaded paths so it matches the on-disk file for non-ASCII logs. A bad `run.zip` is removed from the cache when extraction fails so it cannot poison the cache-first path. Gates: `npm run check` PASS, `npm run build` PASS, `npm test` 241/241 (11 suites), root `npm test` 57/57 — no regressions. The remaining unchecked item (commit message / PR title convention) cannot be changed from the working tree: it requires amending the existing `ee2b5ac` commit and naming the PR, which the pipeline stage is not allowed to do (no commit/amend/push) and which belongs to the repo owner at squash-merge time. Live verification of the filter encoding against a real Forgejo v16 instance also remains a manual smoke-test step (no v16 instance is reachable from this environment; the current git.excelera.net runs v10.0.3). ## Critical (none) ## High - [x] `extensions/forgejo/src/actions.ts:209` — `status`/`event` filters are encoded GitHub-style as `status[]=...&status[]=...`, but the v16 handler reads exact keys via `ctx.FormStrings("status")`/`ctx.FormStrings("event")` (confirmed in the author's own `findings.md`). Go's `url.Values` decodes `status%5B%5D=failure` as the literal key `status[]`, so `FormStrings("status")` returns nothing and the filter **silently no-ops** — `forgejo_action_runs status=[failure]` would return all runs unfiltered. The unit test at `tests/actions.test.ts:125` asserts this same encoding, so the suite cannot catch the mismatch. Suggested fix: send repeated plain keys (`params.append("status", ...)` / `params.append("event", ...)`, matching `collectionFormat: multi`), update the test expectation, update the `status[]`/`event[]` references in `README.md:87` and `docs/DESIGN.md:143,177`, and verify against a real Forgejo v16 instance in the smoke test. ## Medium - [x] `extensions/forgejo/src/index.ts:402` — `actionStatusParam` is `Type.Array(...)` without `Type.Optional`, so `status` is a **required** parameter on `forgejo_action_runs` (confirmed: the schema's `required` array contains `"status"`). The tool cannot list all runs without a status filter, contradicting the API function (`listActionRuns(target, {})` is supported and tested in `actions.test.ts`) and the tool description ("Filters map 1:1 to the API"). The `{}` call in `tests/tools.test.ts:1688` only passes because the typebox mock does not enforce validation. Suggested fix: `status: Type.Optional(actionStatusParam)`. - [x] `extensions/forgejo/src/actions.ts:318` and `:345` — `sizeBytes` is reported as `text.length` (UTF-16 code units), not bytes. For non-ASCII logs (emoji, CJK — exactly the content this extension is built to round-trip), the reported size will not match the on-disk file (`fs.statSync(path).size`) or the `Buffer.from(text, "utf8")` that `writeLog` actually persists. Suggested fix: use `Buffer.byteLength(text, "utf8")` in both the cached-read and downloaded paths. ## Low - [x] `extensions/forgejo/src/actions.ts:391` — `run.zip` is written to the cache before extraction; if extraction fails (zip-bomb rejection or corrupt archive), the bad zip persists in the cache and every subsequent non-`refresh` call re-fails on `extractRunLogsZip(zipPath, ...)` (line 378) until the user knows to pass `refresh: true`. Suggested fix: on extraction failure, remove the just-written zip (or extract before persisting) so a bad artifact cannot poison the cache-first path. - [ ] Commit message convention (AGENTS.md) — the branch commit `ee2b5ac` is `111: Tracking: Forgejo Actions tooling — read-only runs/jobs/logs tools`; AGENTS.md requires `issue-<N>: <summary>` for feature work (e.g. `issue-111: ...`, as used by neighboring commits). Suggested fix: amend the commit message before/at squash-merge, and use the `issue-111: <title>` PR title format when the PR is opened. ## Documentation Updated files: - README.md

david added 3 commits

2026-08-29 04:23:17 +00:00