Repository navigation
Port Forge OpenShell reliability fixes onto current main - #109
sanafayyaz315 wants to merge 6 commits into
Conversation
Port the verified OIDC refresh, USER.md fixture, pinned SAW image, and published_brief gate onto main. Add same-namespace NetworkPolicy templates that match the canonical Pipeline name or app.kubernetes.io/part-of=abevalflow so ad-hoc Pipeline copies no longer time out on gateway preflight. Co-authored-by: Cursor <cursoragent@cursor.com>
Depth-1 --branch fails for bare SHAs; fall back to full clone + checkout so clean-source pins work the same way as the submission revision. Co-authored-by: Cursor <cursoragent@cursor.com>
GuyZivRH
left a comment
There was a problem hiding this comment.
lgtm, merge after confirming harness main includes the OpenClaw fixes (agent-eval-harness#9), since defaults moved off feat/aeh-openshell-openclaw. The port of #105 onto #101/#108 is coherent: OIDC refresh, USER.md fixture, SAW digest pin, prepare SHA clone, and NetworkPolicy/docs are the right fixes. Tests cover the important wiring (#108 LLM param + USER.md before run_aeh.py).
Non-blocking: clarify in the PR text that the “full-brief publication gate” is mostly image/harness, not new pipeline YAML; verify app.kubernetes.io/part-of actually lands on evaluate pods (or add podTemplate labels); update the example PipelineRun off feature-branch pins after merge; optional follow-up on OIDC refresh fail-soft behavior.
Still need the usual post-merge cluster step (oc apply Tekton + one Forge run with the new NetworkPolicies).
kami619
left a comment
There was a problem hiding this comment.
Structural review
Requesting changes because this preserves and adds a lot of incidental complexity where a simpler model is available. The behavior may work in a specific cluster, but the structural bar is not met yet.
1. Collapse the duplicate NetworkPolicy selector model
config/forge-saw/networkpolicy-ci-openshell.yaml:72 and config/forge-saw/networkpolicy-ci-openshell.yaml:145 define the same egress rules twice; only the pod selector differs. The ingress rules also duplicate the same two-selector convention.
The app.kubernetes.io/part-of=abevalflow selector is also broader than "evaluate pods": it grants every similarly labeled pod access to SAW, LiteLLM, MLflow, and MinIO.
Can we select the evaluate TaskRun directly instead? For example, after confirming the injected label in this cluster, use tekton.dev/pipelineTask=evaluate. That would:
- delete the entire duplicate egress policy,
- remove the alternate
part-ofselector, - remove the PipelineRun-label requirement in the example,
- keep the policy scoped to the pods that actually need gateway/service access.
If pipelineTask is not available, please choose one explicit identity and enforce it consistently rather than maintaining two parallel selector modes.
2. Reconcile the egress allowlist with actual step dependencies
The new egress rules use same-namespace pod selectors for LiteLLM and MLflow (config/forge-saw/networkpolicy-ci-openshell.yaml:108), while the example targets services in gz-forge-eval (pipeline/runs/openshell-openclaw-pipelinerun.yaml:51). Unless another policy already grants those paths, this policy can deny the run it is meant to enable.
The same Task also downloads from GitHub and PyPI (pipeline/tasks/phases/evaluate.yaml:1741, pipeline/tasks/phases/evaluate.yaml:1895) and calls the OIDC issuer (pipeline/tasks/phases/evaluate.yaml:1806), but the policy has no explicit route for those dependencies.
Can we make the boundary explicit? Either:
- pre-bake the CLI/Python dependencies and use this as a runtime-only policy, with explicit namespace rules for the services it must reach; or
- model the setup/runtime egress requirements directly, including a documented proxy or explicit destinations.
Shipping an allowlist that does not cover the step's real contract makes the NetworkPolicy harder to reason about and can turn a reliability fix into a new failure mode.
3. Extract OIDC cache refresh from the Evaluate Task
pipeline/tasks/phases/evaluate.yaml:1802 now embeds a background shell daemon and a second Python cache writer inside an already very large Task script.
The initial writer and refresh writer implement the same cache format with different safety properties: the initial path writes directly and then chmods, while refresh uses an atomic replacement. That split is easy to break during future edits.
Can we move this into one tested helper, ideally owned by the harness/OpenShell client? The Task should call the same helper for the initial token and periodic refresh. That would:
- remove the duplicated cache format logic,
- make the initial write atomic too,
- keep this lifecycle concern out of orchestration YAML,
- make the behavior unit-testable.
4. Move the Forge user fixture contract out of the shared Task
pipeline/tasks/phases/evaluate.yaml:2107 hardcodes $SUBMISSION_DIR/fixtures/USER.md, and tests/test_openshell_pipeline_profile.py:97 locks that path into the shared Task contract.
This feels like submission-specific feature logic leaking into a generic Evaluate Task. Can we make the file explicit instead—through a profile parameter or submission-level configuration—so other OpenShell submissions are not implicitly expected to use this directory convention?
5. Pin the harness default
pipeline/pipelines/ci-pipeline-openshell.yaml:104 defaults the harness revision to main, even though this PR adds full-SHA checkout support and the description recommends pinning for reproducible runs.
Can we default to the verified commit SHA (for example 8d58e500d0eb55a3106819ccacf26da6bf7ea166) and leave main as an explicit development override? A moving default makes the reliability path non-reproducible.
6. Simplify the pipeline-repo checkout
pipeline/tasks/phases/prepare.yaml:124 adds another branch-versus-SHA fallback. A direct flow such as git init + git fetch --depth 1 origin "$revision" + detached checkout of FETCH_HEAD supports branches, tags, and SHAs without the nested clone/checkout fallback. If a shared helper is more appropriate, please reuse one canonical checkout helper rather than adding another bespoke variant.
7. Add behavior-level coverage for the new logic
The current tests cover fixture strings and wiring, but not OIDC refresh or checkout-by-SHA. Extracting those paths into helpers would make meaningful tests practical. At minimum, please cover:
- cache refresh writes atomically with mode
0600, - checkout works for a full commit SHA,
- the NetworkPolicy selector matches only the intended TaskRun pods.
Verification performed
python3 -m pytest -q tests/test_openshell_pipeline_profile.py— 6 passed.- Reported CI checks were green at review time.
git diff --checkwas clean.
AI-Attribution: AIA PAI Ce Hin R rits/zai-org/glm-5-3 v1.0
AI-Interpretation: https://aiattribution.github.io/statements/AIA-PAI-Ce-Hin-R-?model=rits%2Fzai-org%2Fglm-5-3
Summary
Port the Forge OpenShell reliability changes from #105 onto current
main, which already contains #101 and #108. This is a replacement path for #105 because its old-base branch currently has merge conflicts.The two original #105 commits were cherry-picked in order. The only cherry-pick conflict was in
tests/test_openshell_pipeline_profile.py; the resolution retains both #108's inference-key test and #105'sUSER.mdfixture test.Changes
USER.mdfixture and use it when staging the Forge evaluation.maininstead of the old feature branch.The
published_briefcheck and its 100% threshold were already present onmain; this PR does not introduce a new publication gate in Pipeline YAML. Full brief publication depends primarily on the pinned SAW image and the OpenClaw execution/brief-reader fixes in the harness. The flow changes here provide the fixture, runtime wiring, and network path needed to exercise that existing gate reliably.The changed Forge evaluation files are identical to #105's head; differences from that head are the existing #101/#108 main-line changes and the retained #108 test. No scorer-development or collector-diagnostic commits are included here.
Pre-merge verification
main. Its head8d58e500d0eb55a3106819ccacf26da6bf7ea166is an ancestor of current harnessmain884c54d782837b8465eb8a38b6193bff02256103(checked 2026-10-08). The OpenShell Pipeline and Evaluate Task default to that repository'smain.forge-nommen, completed podsana-morning-briefing-pr105-single-dgftr-evaluate-podhas bothapp.kubernetes.io/part-of=abevalflowandtekton.dev/pipeline=abevalflow-pipeline-openshell. Its PipelineRun has the same selector labels. Both are accepted by the proposed NetworkPolicy; no additional podTemplate label is needed for this label path.uv run --no-project --with pytest --with pyyaml python -m pytest -q tests/test_openshell_pipeline_profile.py— 6 passed on the original port commit.git -c core.whitespace=cr-at-eol diff --check upstream/main HEADpassed on the original port. GitHub checks must rerun for the latest example update.After merge
Merging updates Git source only. Apply the updated Tekton Pipeline/Tasks and the Forge NetworkPolicies to the target namespace, then run one Forge evaluation to verify the installed resources. Existing SHA-pinned PipelineRuns remain unchanged. OIDC refresh fail-soft behavior can be assessed separately.