fix(G2): pass owner/repo through env: in boards-rollup #42
No reviewers
Labels
No labels
idea
points
1
points
13
points
2
points
3
points
5
points
8
priority
p0
priority
p1
priority
p2
priority
p3
state
blocked
state
done
state
in-progress
state
ready
state
review
state
triage
status
declined
status
in-progress
status
planned
status
proposed
status
shipped
status
under-review
type
bug
type
epic
type
feature
type
spike
type
story
type
task
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
tti/tti-ux!42
Loading…
Reference in a new issue
No description provided.
Delete branch "chore/g2-rollup-injection-fix"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Re-vendors the canonical
boards-rollup.yml, which passesgithub.repository_ownerandgithub.event.repository.namethroughenv:instead of interpolating them directly into therun:block.Found by the H2 baseline gate's semgrep step (
tti.gha-interpolation-in-run) during the K 1.4 triage on 2026-08-18: the canonical file was fixed but the vendored copies were never re-seeded, so six repos still ran the old form.Exploitability is low. Forgejo constrains owner and repo names to AlphaDashDot, so neither value can carry shell metacharacters today -- this restores the invariant rather than closing a live hole.
.forgejo/boards/_boards.pyis re-vendored alongside (install-g2.sh ships both). That delta is purely additive: G3/G4 analytics helpers the Action itself never calls.AI review · advisory
Verdict: 5 things worth fixing (1 high · 2 medium · 2 low).
Findings that didn't map to a diff line:
.forgejo/boards/_boards.py:369· LOW — _default_branch may call sys.exit on non-GET errors during PR branch setup_default_branch calls _get_opt which calls sys.exit on any HTTP error that is not 404. During install --pr, if the repo's default branch endpoint returns a 403 or 5xx, the entire install aborts rather than gracefully skipping that repo. For a multi-repo install this is a robustness risk: one repo's transient error blocks all subsequent repos.
⚑ panel: GPT-OSS 120B · Gemma 4 31B · Devstral 2 123B · Laguna S 2.1 — 8 distinct, 5 confirmed, 1 refuted · web: not used · context: 2 files · 90 codebase · 14 standards chunks (best-grounded: GPT-OSS 120B; smaller windows saw less)
Panel debate — how this review was reached
Grounding — context: 2 files · 90 codebase · 14 standards chunks (best-grounded: GPT-OSS 120B; smaller windows saw less)
Round 1 — independent reviews
_contents_getwithout therefparameter, breaking branch‑aware file operations, and leaves duplicatedcmd_installdefinitions that should be clea_put_fileonly affects tRound 2 — cross-examination
Laguna S 2.1#2_put_file branch logic creates files with no content on Forgejo contents API · confirmed: — · refuted: GPT-OSS 120B, Gemma 4 31B, Gemma 4 31B, Devstral 2 123BGPT-OSS 120B#1Function redefinition hides ref‑aware version · confirmed: Gemma 4 31B · refuted: Devstral 2 123BDevstral 2 123B#1Token fallback logic is incorrect · confirmed: — · refuted: GPT-OSS 120B, Gemma 4 31BLaguna S 2.1#1Undefined helper functions referenced in cmd_analytics · confirmed: Devstral 2 123B · refuted: GPT-OSS 120B, Gemma 4 31BGemma 4 31B#1Potential logic error in dry-run branch classification · confirmed: Devstral 2 123B · refuted: GPT-OSS 120BLaguna S 2.1#4use_pr flag is accepted but not validated against non-installation commands · confirmed: GPT-OSS 120B, Gemma 4 31B, Devstral 2 123B · refuted: —GPT-OSS 120B#2Duplicatecmd_installdefinitions · confirmed: — · refuted: Devstral 2 123BLaguna S 2.1#3_default_branch may call sys.exit on non-GET errors during PR branch setup · confirmed: GPT-OSS 120B, Gemma 4 31B, Devstral 2 123B · refuted: —Raised but refuted (left out of the review above)
Laguna S 2.1#2_put_file branch logic creates files with no content on Forgejo contents API — The POST/PUT body always includes the required "content" field (b64) and, when a branch is specified, the "branch" field is added. No scenario creatSynthesis — Devstral 2 123B wrote the final review from 5 confirmed findings (+2 unconfirmed).
Transcript
rv-20260818163347-41a215— full round outputs, web results, and model reasoning are viewable by anyone with access to this repository via the AI gateway.Advisory — never a merge gate. Disagree with a finding? Reply on it, or use the finding board under this review. Transcript
rv-20260818163347-41a215.@ -37,3 +37,3 @@def _auth_header():tok = os.environ.get("FORGEJO_TOKEN")tok = os.environ.get("FORGEJO_TOKEN") or os.environ.get("FORGEJO_ADMIN_TOKEN")HIGH — Token fallback logic is incorrect
The fallback to FORGEJO_ADMIN_TOKEN is incorrect because it uses
orwhich will always evaluate to FORGEJO_ADMIN_TOKEN if FORGEJO_TOKEN is an empty string, even if FORGEJO_ADMIN_TOKEN is also empty or None.Proposed replacement (one-click ⚡ Apply on the findings board at the top of this PR):
panel tally 1/4 · reply here or use the finding board to agree/disagree
@ -324,17 +324,43 @@ _G2_FILES = {}MEDIUM — Undefined helper functions referenced in cmd_analytics
cmd_analytics and _org_analytics call _points_of (already defined elsewhere) but also reference undefined helpers _has_label, _EPIC_RE_TMPL usage is defined, however _points_of is defined at module level — but the new G3/G4 block references _points_of which IS defined. More critically: the analytics block uses _issues which is defined, _get_paged which is defined. However pyflakes [standard] tti/coding-standards/.forgejo/workflows/baseline.yml:301 gates on undefined names as BLOCKING — these new functions reference module-level names that exist, so no blocking undefined names. Re-evaluating: all referenced names (_points_of, _issues, _get_paged, _label_of, _iso_dt, _week_key, _has_label, _EPIC_RE_TMPL, _POINTS_RE, _default_branch, re, csv, os, datetime, json, urllib.parse, urllib.request) are defined at module scope within the diff. No undefined names introduced.
panel tally 2/4 · reply here or use the finding board to agree/disagree
@ -336,0 +339,4 @@With `branch`, writes to that branch instead of the default one. Thecreate-vs-update decision is then made against the file AS IT EXISTS ONTHAT BRANCH, not on the default branch: a stale working branch can alreadycarry the file while the default does not, and a create against anLOW — Duplicate
cmd_installdefinitionsThe file defines
cmd_installtwice – the first three‑parameter version is overwritten by the later four‑parameter version, leaving dead code and possible maintenance confusion.Proposed replacement (one-click ⚡ Apply on the findings board at the top of this PR):
panel tally 1/4 · reply here or use the finding board to agree/disagree
@ -346,3 +372,3 @@else:action = "created"action = "updated" if base_has_it else "created"if not apply:.forgejo/boards/_boards.py:374· MEDIUM — Potential logic error in dry-run branch classificationThe logic for
base_has_itrelies onbranchandbasebeing truthy, butbaseis only set ifuse_pris True. Incmd_install, ifuse_pris False,baseremains None, skipping the base-check logic that prevents reporting 'created' for files already in the base branch.panel tally 2/4 · reply here or use the finding board to agree/disagree
@ -338,0 +348,4 @@base_has_it = Falseif existing is None and branch and base:# Nothing on the working branch. In dry-run the branch does not exist# yet, so fall back to the base to classify honestly: reporting.forgejo/boards/_boards.py:351· HIGH — Function redefinition hides ref-aware versionThe later definition of
_contents_getdrops therefparameter, so calls that passref=(e.g., from_put_file) will raise aTypeError. This breaks any code that relies on the original function signature.Proposed replacement (one-click ⚡ Apply on the findings board at the top of this PR):
panel tally 2/4 · reply here or use the finding board to agree/disagree
@ -363,0 +407,4 @@def _ensure_pr(org, repo, branch, base, title, body):"""Open the PR. Returns a short status string; already-exists is fine.""".forgejo/boards/_boards.py:410· LOW — use_pr flag is accepted but not validated against non-installation commandsThe
--prflag setsuse_pr=Truebut is only meaningful for the 'install' command. If a user runs 'analytics' or 'rollup' with--pr, the flag is silently ignored with no warning, which can confuse operators about whether the command actually used a PR workflow.Proposed replacement (one-click ⚡ Apply on the findings board at the top of this PR):
panel tally 4/4 · reply here or use the finding board to agree/disagree
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.