Skip to content

Fix: drop redundant "browser" prune from scripts/check_license.sh - #594

Open
AmaadMartin wants to merge 1 commit into
mainfrom
fix/check-license-drop-redundant-browser-prune
Open

Fix: drop redundant "browser" prune from scripts/check_license.sh#594
AmaadMartin wants to merge 1 commit into
mainfrom
fix/check-license-drop-redundant-browser-prune

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 3, 2026

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):
    No existing issue; description below.
  2. Or, if no issue exists, describe the change:
    Problem: scripts/check_license.sh prunes four directory names while building its candidate file list, and one of them is dead code:
FILES=$(find . -type d \( -name "node_modules" -o -name "dist" -o -name ".git" -o -name "browser" \) -prune -o \
               -type f \( -name "*.js" -o -name "*.ts" \) -print)

The -name "browser" clause was present in the script's first revision ("Add license check workflow", 97d92bbd) and has never been touched since. It was there to keep the downloaded ADK Web assets out of the license-header check. Those assets are extracted by ensureBrowserAssets() in dev/build.js to dev/dist/browser.

find evaluates -prune against each directory's basename as it descends. Descending from the repo root it reaches dev/dist first, prunes it on the dist clause, and therefore never descends into dev/dist/browser at all. The dist prune alone already excludes every ADK Web asset, so the browser clause excludes nothing that the surviving prunes do not.

Leaving it costs two things: a future reader believes the clause is load-bearing, and — more importantly — it is redundant, not a universal no-op. A directory named browser outside a pruned subtree (for example a first-party dev/src/browser) would be silently exempted from the license check, which is the exact opposite of what the check exists to do.

Solution: Delete the single -o -name "browser" clause. One file, one line; every other token in the script — the shebang, the banners, the Perl slurp regex, the MISSING_FILES accumulation, the exit codes, the second line's indentation and escaping, and the file mode (100644) — is byte-identical.

Deliberately not included in this PR, to keep the diff at one line:

  • The script reports success when find itself errors out (it never inspects find's exit status, so a mangled expression yields an empty list and a false green). That is a real but separate defect and is out of scope here; adding set -euo pipefail or a find-exit-status guard would turn a one-line deletion into a behavior change.
  • eslint.config.js and .prettierignore still carry stale dev/src/browser ignore entries. They are untouched here — a separate change removes those, and editing them in this PR would conflict.

Collision check: before starting, I listed all 493 open PRs on the fork (gh pr list --repo AmaadMartin/adk-js --state open --limit 1000) and scanned every fork branch's history for commits touching this file (git log fork/main..<branch> -- scripts/check_license.sh over all remote refs). No open PR and no fork branch modifies scripts/check_license.sh; the only commit in its history is the one that introduced it. The nearest neighbours were checked by file list and do not overlap: #454 touches only .prettierignore and eslint.config.js, #553 touches the build-time license banner in core/build.js/integrations/build.js, and #426 touches eslint.config.js plus code files.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.

Unit Tests:
[x] I have added or updated unit tests for my change.

No unit test file was added, and this is a deliberate, stated choice rather than an omission. The change adds zero lines of executable code — it deletes one clause from a Bash find expression. The repo has no shell-script test harness: every project in vitest.config.ts (unit:core, unit:dev, unit:integrations, integration, e2e, cross-language) includes only **/*_test.ts. A Vitest spec that shells out to check_license.sh would be a multi-minute, build- and network-dependent test guarding a line that CI already executes on every push and PR. The standing regression gate is the existing License Header Check workflow, which is this script's only caller (.github/workflows/license-check.yml:18, bash scripts/check_license.sh); nothing else in the repo references it and no package.json script invokes it. Verification below is therefore behavioral and was executed by hand.

[x] All unit tests pass locally.

Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

All of the following were run from the repo root on a fully built tree (npm install && npm run build, which downloads adk-web-browser.zip and extracts it), because a built tree is the only case where the pruned directory actually exists.

1. Premise — nothing named browser exists outside a pruned subtree:

$ find . -type d -name browser -not -path './node_modules/*'
./dev/dist/browser          # only hit, and it is inside dev/dist
$ git ls-files | grep -i browser
                            # no output: no tracked path contains "browser"
$ find . -type d -name browser | wc -l
105                         # 104 under node_modules (pruned at top level) + dev/dist/browser
$ find dev/dist/browser -name '*.js' | wc -l
101                         # unlicensed vendor assets

2. Behavior is unchanged — stdout, exit status, and the file list:

$ bash scripts/check_license.sh   # before
🔍 Checking for license headers...
✅ All files have the correct license header.
exit=0
$ bash scripts/check_license.sh   # after
🔍 Checking for license headers...
✅ All files have the correct license header.
exit=0
$ diff /tmp/before.txt /tmp/after.txt
                            # identical; stderr empty on both runs

File lists compared directly (4-prune before vs 3-prune after, both sorted):

$ diff /tmp/before_list.txt /tmp/after_list.txt
                            # identical
$ wc -l < /tmp/after_list.txt
545                         # non-empty, out of 35968 .js/.ts files before pruning

The list count matters as much as the empty diff: an empty list would also produce a passing script, so a green run alone does not prove the find expression survived the edit. 545 paths, unchanged, does.

3. CI-equivalent run on a bare checkout. The workflow runs on actions/checkout with no build step, so dev/dist does not exist there. Reproduced by cloning to a scratch dir and copying in the edited script:

$ ls -d node_modules dist dev/dist
ls: cannot access 'node_modules': No such file or directory
ls: cannot access 'dist': No such file or directory
ls: cannot access 'dev/dist': No such file or directory
$ bash scripts/check_license.sh
🔍 Checking for license headers...
✅ All files have the correct license header.
exit=0
$ find . -type d \( -name "node_modules" -o -name "dist" -o -name ".git" \) -prune -o \
       -type f \( -name "*.js" -o -name "*.ts" \) -print | wc -l
545

4. Mutation proof A — the surviving prunes are load-bearing. Dropping dist as well (i.e. mutating the line further than this PR does) makes the assets reappear:

$ find . -type d \( -name "node_modules" -o -name ".git" \) -prune -o \
       -type f \( -name "*.js" -o -name "*.ts" \) -print | grep -c 'dev/dist/browser'
101

So those 101 files genuinely lack license headers and genuinely need a prune — and dist is the clause providing it. Reverted immediately.

5. Mutation proof B — the deleted clause was not a no-op in the dangerous direction. This is the proof that the change is observable at all, since proofs 2–4 are all equality checks. I created an unlicensed first-party source file at dev/src/browser/probe.ts (a browser directory outside any pruned subtree) and ran both versions of the script against it:

# old script, with the "browser" prune — silently exempts it:
🔍 Checking for license headers...
✅ All files have the correct license header.
OLD exit=0

# new script, clause removed — catches it:
🔍 Checking for license headers...
❌ Missing or invalid license header: ./dev/src/browser/probe.ts
------------------------------------------------
Error: Some files are missing the required license header.
NEW exit=1

The old script's false green on an unlicensed first-party file is the gap this PR closes. The probe file was deleted afterwards and is not part of the diff.

6. Diff hygiene:

$ git diff fork/main --stat
 scripts/check_license.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
$ git status --short
                            # clean; eslint.config.js, .prettierignore and
                            # package-lock.json are unmodified

(npm install did not perturb package-lock.json.)

7. CI. check-license (the workflow that runs this script — the direct regression gate) passed, and the real test jobs run-tests and run-tests on ubuntu-latest, macos-latest and windows-latest all passed. On the first attempt macos-latest failed with a timeout in tests/integration/app_loader/app_loader_test.ts > "should discover apps vs agents across directories and standalone files"; that is a known intermittent flake on the macOS/Windows runners, is unrelated to a one-line shell-script edit that no TypeScript test loads, and the job passed on re-run with no code change.

Checklist

[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.

find evaluates -prune against each directory's basename as it descends, so
dev/dist is pruned before dev/dist/browser is ever reached. The dist prune
alone already excludes the vendored ADK Web assets that the browser clause
was meant to skip, making the clause dead for every directory the repository
contains or its build produces.

Verified on a fully built tree (node_modules installed, dev/dist/browser
populated with 101 unlicensed vendor .js files): the four-prune and
three-prune file lists are byte-identical at 545 paths, and the script's
stdout and exit status are unchanged.

Removing the clause also closes a gap: a first-party source directory named
browser outside a pruned subtree would previously have been exempted from the
license check, which is the opposite of the check's purpose.
AmaadMartin pushed a commit that referenced this pull request Aug 6, 2026
…7) (#594)

* feat(tools): add FunctionTool require_confirmation (human-in-the-loop approval)

Part 7/9 of the feature/workflows split.

- tools/function_tool: a `requireConfirmation` option so a FunctionTool pauses
  for human approval before executing.
- agents/processors/request_confirmation_llm_request_processor: handles the
  confirmation request/resume round-trip for such tools.

This tool-approval HITL is independent of the workflow engine (it works for any
FunctionTool), so it is a small, self-contained slice.

Tests: tools/function_tool_confirmation_test (5). Full core suite green (2481),
docs:check + tsc clean.

* fix(tools): gate and harden plain-text tool confirmation (PR #594)

Addresses the security/API review on FunctionTool require_confirmation:

- The plain-text confirmation fallback no longer runs on every LlmAgent
  invocation. It is now opt-in via a new `RunConfig.plainTextToolConfirmation`
  flag (default off), which the interactive `adk run` CLI sets — so on a web/API
  surface an ordinary chat message is never silently reinterpreted as a tool-gate
  decision. The structured FunctionResponse path is unchanged.
- Harden the fallback itself: resolve only the SINGLE most-recent pending
  confirmation (never a broadcast across every unanswered gate), require the
  reply to IMMEDIATELY follow the request (no intervening user turn), and treat
  unrecognized text as NO decision — the gate stays pending instead of being
  silently denied (only explicit negatives deny).
- Extract a `RequireConfirmation<TParameters>` type with a `toolContext` (not
  snake_case `tool_context`) parameter, reuse it for both the option and the
  field, and export it from common.ts.
- Correct the `requireConfirmation` doc: the HITL gate is enforced on the
  LlmAgent path; a workflow ToolNode does not yet route through it (it returns
  the "requires confirmation" error as node output rather than pausing).
- Inline the redundant `await` in runAsync and drop the stale comment.

* test(tools): cover the confirmation resume round-trip (PR #594)

- Add end-to-end tests that drive a session event list back through
  RequestConfirmationLlmRequestProcessor with a real LlmAgent + real
  FunctionTool (no mocks) and assert the original tool is actually re-invoked
  with the right decision — the step where an id mismatch on resume would show
  up, and the first coverage of the plain-text fallback: opt-in gating,
  single-gate binding, unrecognized-text-stays-pending, and no cross-gate
  broadcast.
- Replace the `agent: ... as never` fixture with a real LlmAgent instance so it
  breaks if InvocationContext's contract changes.
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.

1 participant