fix(browser_harness): deduplicate env loading and add repo automation#473
fix(browser_harness): deduplicate env loading and add repo automation#473giattijunior wants to merge 10 commits into
Conversation
- Monthly Renovate run (2nd of month, 02:00 BRT) - minimumReleaseAge: 14 days (supply-chain protection) - Python deps via pep621 manager - Major bumps require manual review
admin.py and daemon.py each defined _load_env and _load_env_file identically (admin.py: 12-30, daemon.py: 10-28). Both now import from helpers.py, which is the canonical source (was already exporting both via module-level _load_env() side effect). CI: - .github/workflows/dupehound.yml: scan (slop score) and check (--diff against base) jobs. check is continue-on-error for now; promote to gate after FP rate <10% over ~10 PRs. Slop score: 0.9% -> 0.0% (4 -> 0 clusters, 18 lines eliminated). 40/40 unit tests pass.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05e7e005ff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| from . import _ipc as ipc | ||
| from . import auth | ||
| from . import paths | ||
| from .helpers import _load_env |
There was a problem hiding this comment.
This import executes the whole helpers module, and helpers.py:508 unconditionally runs _load_agent_helpers(). In any install where agent-workspace/agent_helpers.py contains task-specific top-level code or an import that is only valid during helper scripts, simply importing browser_harness.admin for commands like --doctor, --reload, or daemon management now raises before the admin command can run; previously env loading in admin/daemon did not load agent helpers. Keep env parsing in a side-effect-free module or avoid importing helpers here.
Useful? React with 👍 / 👎.
| name: Block new duplicates | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 5 | ||
| continue-on-error: true |
There was a problem hiding this comment.
check is the only job running dupehound check, but job-level continue-on-error: true makes its failure non-fatal for the workflow. In PRs touching src/** or tests/** that introduce duplicates, this contradicts the documented blocking behavior in README/AGENTS and lets the workflow pass as long as the separate scan job succeeds; remove this flag or keep it only on non-gating reporting jobs.
Useful? React with 👍 / 👎.
| uses: renovatebot/github-action@v41 | ||
| with: | ||
| configurationFile: .github/renovate.json5 | ||
| token: ${{ secrets.GITHUB_TOKEN }} |
There was a problem hiding this comment.
For the scheduled Renovate workflow, ${{ secrets.GITHUB_TOKEN }} makes dependency PRs come from the workflow token; Renovate's GitHub Action docs note that this token is too restrictive and PRs made with it do not trigger PR/push CI events (https://github.com/renovatebot/github-action#token). That means the monthly update PRs, including workflow updates covered by the github-actions rule, can arrive without the checks this repo relies on; use a PAT or GitHub App token secret with the required workflow permissions instead.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 issues found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/dupehound.yml">
<violation number="1" location=".github/workflows/dupehound.yml:22">
P2: Job named "Block new duplicates" sets `continue-on-error: true`, which means the job never blocks CI — the name contradicts the actual enforcement behavior.</violation>
<violation number="2" location=".github/workflows/dupehound.yml:30">
P1: Unpinned binary download from `releases/latest` without checksum verification introduces supply-chain risk — a new release could silently change behavior or introduce malicious code.</violation>
<violation number="3" location=".github/workflows/dupehound.yml:39">
P2: `HEAD~1` only goes back one commit, which may miss duplicates introduced in earlier commits of a multi-commit push to `main`.</violation>
</file>
<file name="README.md">
<violation number="1" location="README.md:74">
P2: Describes dupehound as "blocking new duplication" but the workflow uses `continue-on-error: true` on the "Block new duplicates" job, making it non-blocking — the docs overstate enforcement.</violation>
</file>
<file name=".github/workflows/renovate.yml">
<violation number="1" location=".github/workflows/renovate.yml:12">
P3: No explicit `permissions:` block. Add one to restrict the default token scope by default. Even though the GITHUB_TOKEN issue above makes a PAT the right fix, adding a `permissions:` block aligns with the repo's existing convention (dupehound.yml, release.yml both declare explicit permissions).</violation>
<violation number="2" location=".github/workflows/renovate.yml:12">
P2: Mutable action tags (`@v4`, `@v41`) are used instead of pinned SHAs. While `pinDigests: true` in renovate.json5 will auto-pin these on first run, the initial state is unpinned. Pin them pre-emptively to match the supply-chain hardening pattern already established by `dupehound.yml`.</violation>
<violation number="3" location=".github/workflows/renovate.yml:18">
P1: Using `secrets.GITHUB_TOKEN` with the Renovate action prevents CI workflows from running on Renovate-created PRs, and the token lacks the `workflow` scope required by the `github-actions` manager. The Renovate docs explicitly warn against this — it will silently drop all CI gating on dependency updates.</violation>
</file>
<file name="src/browser_harness/admin.py">
<violation number="1" location="src/browser_harness/admin.py:14">
P1: Importing `_load_env` from `.helpers` triggers execution of the entire `helpers` module at import time. If `helpers.py` has top-level side effects (e.g., calling `_load_agent_helpers()` unconditionally), then simply importing `browser_harness.admin` for lightweight commands like `--doctor` or `--reload` may raise before the admin command runs. The previous implementation kept env parsing local and side-effect-free. Consider either guarding the side-effectful code in `helpers.py` behind `if __name__` / a lazy call, or moving `_load_env` into a minimal, side-effect-free utility module.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| fetch-depth: 0 | ||
| - name: Install dupehound | ||
| run: | | ||
| curl -sL https://github.com/Rafaelpta/dupehound/releases/latest/download/dupehound-x86_64-unknown-linux-gnu.tar.gz | tar xz |
There was a problem hiding this comment.
P1: Unpinned binary download from releases/latest without checksum verification introduces supply-chain risk — a new release could silently change behavior or introduce malicious code.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/dupehound.yml, line 30:
<comment>Unpinned binary download from `releases/latest` without checksum verification introduces supply-chain risk — a new release could silently change behavior or introduce malicious code.</comment>
<file context>
@@ -0,0 +1,58 @@
+ fetch-depth: 0
+ - name: Install dupehound
+ run: |
+ curl -sL https://github.com/Rafaelpta/dupehound/releases/latest/download/dupehound-x86_64-unknown-linux-gnu.tar.gz | tar xz
+ sudo mv dupehound /usr/local/bin/
+ - name: Block new duplicates vs base
</file context>
| uses: renovatebot/github-action@v41 | ||
| with: | ||
| configurationFile: .github/renovate.json5 | ||
| token: ${{ secrets.GITHUB_TOKEN }} |
There was a problem hiding this comment.
P1: Using secrets.GITHUB_TOKEN with the Renovate action prevents CI workflows from running on Renovate-created PRs, and the token lacks the workflow scope required by the github-actions manager. The Renovate docs explicitly warn against this — it will silently drop all CI gating on dependency updates.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/renovate.yml, line 18:
<comment>Using `secrets.GITHUB_TOKEN` with the Renovate action prevents CI workflows from running on Renovate-created PRs, and the token lacks the `workflow` scope required by the `github-actions` manager. The Renovate docs explicitly warn against this — it will silently drop all CI gating on dependency updates.</comment>
<file context>
@@ -0,0 +1,20 @@
+ uses: renovatebot/github-action@v41
+ with:
+ configurationFile: .github/renovate.json5
+ token: ${{ secrets.GITHUB_TOKEN }}
+ env:
+ LOG_LEVEL: info
</file context>
| from . import _ipc as ipc | ||
| from . import auth | ||
| from . import paths | ||
| from .helpers import _load_env |
There was a problem hiding this comment.
P1: Importing _load_env from .helpers triggers execution of the entire helpers module at import time. If helpers.py has top-level side effects (e.g., calling _load_agent_helpers() unconditionally), then simply importing browser_harness.admin for lightweight commands like --doctor or --reload may raise before the admin command runs. The previous implementation kept env parsing local and side-effect-free. Consider either guarding the side-effectful code in helpers.py behind if __name__ / a lazy call, or moving _load_env into a minimal, side-effect-free utility module.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/browser_harness/admin.py, line 14:
<comment>Importing `_load_env` from `.helpers` triggers execution of the entire `helpers` module at import time. If `helpers.py` has top-level side effects (e.g., calling `_load_agent_helpers()` unconditionally), then simply importing `browser_harness.admin` for lightweight commands like `--doctor` or `--reload` may raise before the admin command runs. The previous implementation kept env parsing local and side-effect-free. Consider either guarding the side-effectful code in `helpers.py` behind `if __name__` / a lazy call, or moving `_load_env` into a minimal, side-effect-free utility module.</comment>
<file context>
@@ -11,6 +11,7 @@
from . import _ipc as ipc
from . import auth
from . import paths
+from .helpers import _load_env
</file context>
| if [ -n "$PR_BASE" ]; then | ||
| dupehound check --diff "origin/$PR_BASE" . | ||
| else | ||
| dupehound check --diff HEAD~1 . |
There was a problem hiding this comment.
P2: HEAD~1 only goes back one commit, which may miss duplicates introduced in earlier commits of a multi-commit push to main.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/dupehound.yml, line 39:
<comment>`HEAD~1` only goes back one commit, which may miss duplicates introduced in earlier commits of a multi-commit push to `main`.</comment>
<file context>
@@ -0,0 +1,58 @@
+ if [ -n "$PR_BASE" ]; then
+ dupehound check --diff "origin/$PR_BASE" .
+ else
+ dupehound check --diff HEAD~1 .
+ fi
+
</file context>
| name: Block new duplicates | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 5 | ||
| continue-on-error: true |
There was a problem hiding this comment.
P2: Job named "Block new duplicates" sets continue-on-error: true, which means the job never blocks CI — the name contradicts the actual enforcement behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/dupehound.yml, line 22:
<comment>Job named "Block new duplicates" sets `continue-on-error: true`, which means the job never blocks CI — the name contradicts the actual enforcement behavior.</comment>
<file context>
@@ -0,0 +1,58 @@
+ name: Block new duplicates
+ runs-on: ubuntu-latest
+ timeout-minutes: 5
+ continue-on-error: true
+ steps:
+ - name: Checkout
</file context>
| continue-on-error: true | |
| continue-on-error: false |
| GitHub automation also runs alongside local development: | ||
|
|
||
| - Renovate opens monthly dependency update PRs against `main` using `.github/renovate.json5`. | ||
| - `dupehound` runs on PRs and `main` pushes that touch `src/**` or `tests/**`, blocking new duplication and publishing a repo slop score in the workflow summary. |
There was a problem hiding this comment.
P2: Describes dupehound as "blocking new duplication" but the workflow uses continue-on-error: true on the "Block new duplicates" job, making it non-blocking — the docs overstate enforcement.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At README.md, line 74:
<comment>Describes dupehound as "blocking new duplication" but the workflow uses `continue-on-error: true` on the "Block new duplicates" job, making it non-blocking — the docs overstate enforcement.</comment>
<file context>
@@ -68,6 +68,11 @@ PY
+GitHub automation also runs alongside local development:
+
+- Renovate opens monthly dependency update PRs against `main` using `.github/renovate.json5`.
+- `dupehound` runs on PRs and `main` pushes that touch `src/**` or `tests/**`, blocking new duplication and publishing a repo slop score in the workflow summary.
+
## Contributing
</file context>
| - `dupehound` runs on PRs and `main` pushes that touch `src/**` or `tests/**`, blocking new duplication and publishing a repo slop score in the workflow summary. | |
| - `dupehound` runs on PRs and `main` pushes that touch `src/**` or `tests/**`, flagging new duplication and publishing a repo slop score in the workflow summary. |
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
P2: Mutable action tags (@v4, @v41) are used instead of pinned SHAs. While pinDigests: true in renovate.json5 will auto-pin these on first run, the initial state is unpinned. Pin them pre-emptively to match the supply-chain hardening pattern already established by dupehound.yml.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/renovate.yml, line 12:
<comment>Mutable action tags (`@v4`, `@v41`) are used instead of pinned SHAs. While `pinDigests: true` in renovate.json5 will auto-pin these on first run, the initial state is unpinned. Pin them pre-emptively to match the supply-chain hardening pattern already established by `dupehound.yml`.</comment>
<file context>
@@ -0,0 +1,20 @@
+ runs-on: ubuntu-latest
+ steps:
+ - name: Checkout
+ uses: actions/checkout@v4
+
+ - name: Self-hosted Renovate
</file context>
| @@ -0,0 +1,20 @@ | |||
| name: Renovate | |||
There was a problem hiding this comment.
P3: No explicit permissions: block. Add one to restrict the default token scope by default. Even though the GITHUB_TOKEN issue above makes a PAT the right fix, adding a permissions: block aligns with the repo's existing convention (dupehound.yml, release.yml both declare explicit permissions).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/renovate.yml, line 12:
<comment>No explicit `permissions:` block. Add one to restrict the default token scope by default. Even though the GITHUB_TOKEN issue above makes a PAT the right fix, adding a `permissions:` block aligns with the repo's existing convention (dupehound.yml, release.yml both declare explicit permissions).</comment>
<file context>
@@ -0,0 +1,20 @@
+ runs-on: ubuntu-latest
+ steps:
+ - name: Checkout
+ uses: actions/checkout@v4
+
+ - name: Self-hosted Renovate
</file context>
1 participant
What Changed
src/browser_harness/admin.pyandsrc/browser_harness/daemon.pyto reuse the shared_load_envhelper, removing duplicated env-loading code and fixing the admin loader shadowing issue..github/with Renovate config/workflow, dependency review support, and adupehoundworkflow for duplicate detection in CI.README.md,agent-workspace/SMOKE.md,AGENTS.md, and.no-mistakes.yamlto match the new automation flow.Risk Assessment
Testing
Exercitei a suíte unitária relevante via
uv run, validei o entrypoint real com./browser-harness --doctore gerei evidência parseando os novos workflows/configs; o resultado foi limpo para a mudança proposta, com o doctor mostrando apenas ausência de daemon/conexão de browser ativos neste ambiente de teste.Evidence: browser-harness doctor transcript
Evidence: workflow and renovate config summary
Evidence: focused daemon/admin regression test transcript
Pipeline
Updates from git push no-mistakes
⏭️ **intent** - skipped
✅ No issues found.
🔧 **Rebase** - 2 issues found → auto-fixed ✅
src/browser_harness/admin.py- merge conflict rebasing onto origin/mainsrc/browser_harness/daemon.py- merge conflict rebasing onto origin/main🔧 Fix applied.
✅ Re-checked - no issues remain.
⏭️ **Review** - skipped
.github/workflows/dupehound.yml:22-continue-on-error: truemakes the "Block new duplicates" job non-blocking, so PRs with new duplication still pass the workflow. If the intent is to gate regressions, this setting defeats the check entirely..github/workflows/dupehound.yml:30- The workflow downloads and executesdupehoundfromreleases/latestwith no version pin or checksum verification. That makes CI non-reproducible and creates a supply-chain exposure: any upstream release change immediately alters what runs on every PR..github/workflows/renovate.yml:12- This workflow reintroduces mutable GitHub Action tags (actions/checkout@v4,renovatebot/github-action@v41) after the repo already added SHA pinning. Those tags can move without a PR in this repo, weakening the supply-chain hardening that earlier CI changes established.✅ **Test** - passed
✅ No issues found.
uv run --python 3.12 --with pytest pytest -q tests/unituv run --python 3.12 --with pytest pytest -q tests/unit/test_daemon.py tests/unit/test_admin.pymkdir -p /var/folders/h_/qkglyg0x5rg5vd2s3y84dh9w0000gn/T/no-mistakes-evidence/01KVZBEFMX9X5MEDS2PQHQ9N3Q && ./browser-harness --doctor | tee /var/folders/h_/qkglyg0x5rg5vd2s3y84dh9w0000gn/T/no-mistakes-evidence/01KVZBEFMX9X5MEDS2PQHQ9N3Q/browser-harness-doctor.txtuv run --python 3.12 --with pyyaml --with json5 python - <<'PY' > /var/folders/h_/qkglyg0x5rg5vd2s3y84dh9w0000gn/T/no-mistakes-evidence/01KVZBEFMX9X5MEDS2PQHQ9N3Q/workflow-config-summary.txt ... PYgit diff 7594909e7963c9ba328e39cc79e9f20ff94b2a82..8d5b3b56bc94735c90ef52820c11da8b8d40d4e4 -- .github/workflows/dupehound.yml .github/workflows/renovate.yml .github/renovate.json5 .no-mistakes.yaml agent-workspace/SMOKE.md src/browser_harness/admin.py src/browser_harness/daemon.py✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.
Summary by cubic
Deduplicated env loading in the browser harness by reusing the shared
_load_env, fixing the admin shadowing bug and ensuring consistent env init. Added repo automation with monthly Renovate anddupehoundCI to surface duplication and a slop score.Refactors
src/browser_harness/admin.pyandsrc/browser_harness/daemon.pynow import_load_envfromhelpers; removed duplicate loaders.New Features
main(14-day minimum age; majors require manual review).dupehoundworkflow on PRs andmainpushes touchingsrc/**ortests/**; reports new duplication and posts a slop score (non-blocking for now).Written for commit 05e7e00. Summary will update on new commits.