Skip to content

Port Forge OpenShell reliability fixes onto current main - #109

Open
sanafayyaz315 wants to merge 6 commits into
RHEcosystemAppEng:mainfrom
sanafayyaz315:sana/forge-eval-reliability-main-2026-10-08
Open

sanafayyaz315 wants to merge 6 commits into
RHEcosystemAppEng:mainfrom
sanafayyaz315:sana/forge-eval-reliability-main-2026-10-08

Conversation

@sanafayyaz315

@sanafayyaz315 sanafayyaz315 commented Oct 8, 2026 •

Copy link
Copy Markdown
Collaborator

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's USER.md fixture test.

Changes

The published_brief check and its 100% threshold were already present on main; 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

  • GuyZivRH/agent-eval-harness#9 merged into harness main. Its head 8d58e500d0eb55a3106819ccacf26da6bf7ea166 is an ancestor of current harness main 884c54d782837b8465eb8a38b6193bff02256103 (checked 2026-10-08). The OpenShell Pipeline and Evaluate Task default to that repository's main.
  • In forge-nommen, completed pod sana-morning-briefing-pr105-single-dgftr-evaluate-pod has both app.kubernetes.io/part-of=abevalflow and tekton.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.
  • Global pre-commit hook passed on all PR commits; git -c core.whitespace=cr-at-eol diff --check upstream/main HEAD passed 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.

tarun-etikala and others added 3 commits October 8, 2026 13:24
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
GuyZivRH previously approved these changes Oct 8, 2026

@GuyZivRH GuyZivRH left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 kami619 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-of selector,
  • 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 --check was 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

@sanafayyaz315
sanafayyaz315 requested a review from kami619 October 9, 2026 11:22

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants