Decide keyword call banner and secret log suppression in run_keyword - #5171
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Moves banner and secret-log decisions into run_keyword, making behavior alias- and translation-safe.
Changes:
- Resolves keywords by Python function and masks secret banner arguments.
- Adds nested logging suppression and enriched call-stack data.
- Adds unit and acceptance coverage.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
Browser/browser.py |
Implements banner and secret handling. |
Browser/utils/data_types.py |
Extends keyword stack entries. |
Browser/keywords/playwright_state.py |
Filters trace-group arguments. |
utest/test_keyword_hooks.py |
Adds hook unit tests. |
atest/test/01_Browser_Management/keyword_hooks.robot |
Adds acceptance scenarios. |
atest/test/01_Browser_Management/keyword_banner.py |
Adds cross-browser banner inspection. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
The keyword call banner and the secret log suppression were decided in the listener hooks, filtered by comparing attrs["libname"] against the literal "Browser" and attrs["kwname"] against English keyword names. Both comparisons fail for an aliased import and for every translated keyword, which silently disabled the banner, the muting of Take Screenshot and Get Page Source, and the log suppression. Move both decisions into run_keyword, which by construction only sees this library's keywords, and base them on the resolved Python function instead of the displayed name. All three libname comparisons are gone. The listener keeps what genuinely applies to every keyword: the call stack entry that Playwright trace groups replay, the trace group for TracingGroupMode.Full and the screenshot on test timeout in a teardown. The call stack entry now also carries the keyword name and the source arguments, because the banner shows the call as it was written while run_keyword only sees converted values. Arguments that map to a parameter named secret are masked before the banner resolves variables, so a secret can no longer be painted into the page, the video and the trace. The log suppression counts its nesting depth instead of overwriting a single slot. One deliberate behaviour change: Robot Framework logs its own Arguments trace and the keyword failure outside run_keyword, so those are no longer suppressed for secret keywords. Only what the library logs during the keyword body still is. The secret value itself appears in neither case. Fixes #5154 Fixes #5169 Fixes #5170 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SuiwgM8Rt9kd6K7TBtFMDz
Masking maps the source arguments of a keyword call onto its parameters
by position and by name. A collection expansion is a single source cell
carrying several arguments at once, so it cannot be mapped, and
resolving it printed the whole collection into the banner:
Fill Secret &{arguments}
-> Fill Secret {'selector': 'css=input#username', 'secret': 'hunter2'}
Fill Secret @{arguments}
-> Fill Secret ['css=input#username', 'hunter2']
Variables are no longer resolved at all for a keyword that takes a
secret, which removes the whole class instead of the two known forms.
Every other keyword keeps resolving its variables, with a test that
guards it.
Plugin keywords gain coverage. The banner already reached them, because
a plugin keyword reports the Browser library as its owner, but the
secret detection used to compare the keyword name against the word
secret. A plugin keyword that takes a secret under a name not
containing that word had no protection at all. Detection now reads the
argument specification, and `Plugin Login With Credentials` in the
example plugin proves it end to end.
The shared banner reader moved to atest/library/banner.py, since two
suites read the injected style element now.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SuiwgM8Rt9kd6K7TBtFMDz
Detect a secret argument by its Secret type annotation as well as by its name. The name-only check masked Fill Secret and Type Secret, but Create Credential carries its secrets in privateKey and publicKey and was painting them into the banner in clear text. Also: track which parameter slots a masking pass still owns instead of a single counter, so a keyword with several secret-typed arguments masks each of them; move the initial log suppression into run_keyword's try block so the suppress/restore pairing does not depend on nothing raising in between; log a banner failure instead of swallowing it silently; and remove comments and docstrings that only restated what the code or the test name already said. Fixes #5172 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SuiwgM8Rt9kd6K7TBtFMDz
Snooz82
force-pushed
the
fix/keyword_hooks_in_run_keyword
branch
from
August 18, 2026 17:31
6911528 to
8f87fb0
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
Suppressed comments (1)
Browser/browser.py:1223
- The name-based rule only examines
get_keyword_types(), which omits unannotated parameters (the repository'sspy_untypedand JS-extension coverage confirm such keywords have no type entries). Consequently, a supported plugin keyword such asdef login(self, secret)is classified as non-secret: its body logs are not suppressed and a literal secret can be painted in the banner. Derive thesecretname rule fromget_keyword_arguments()and use the types mapping only for detectingSecretannotations; add an unannotated-plugin case to the tests.
return {
argument
for argument, annotation in argument_types.items()
if argument == SECRET_ARGUMENT
or annotation is Secret
The name rule read from get_keyword_types, which only lists annotated parameters, so a plugin keyword like `def login(self, selector, secret)` with no type hints produced an empty type map and its secret was neither suppressed nor masked. The earlier plugin test masked this by annotating its secret argument. The name rule now reads from get_keyword_arguments, which lists every parameter, and the type map is used only for the Secret annotation. Covered by an unannotated plugin keyword in both the unit and the acceptance tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SuiwgM8Rt9kd6K7TBtFMDz
Adds a plugin secret keyword without type hints to plugin.robot, using the Get Keyword Call Banner Text helper. Against the earlier type-map-only detection the banner shows the plugin secret in clear text, so the case guards the name rule end to end alongside the unit test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SuiwgM8Rt9kd6K7TBtFMDz
Snooz82
force-pushed
the
fix/keyword_hooks_in_run_keyword
branch
from
August 18, 2026 21:27
d239082 to
be99c39
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
The keyword call banner and the secret log suppression were decided in the listener hooks and filtered with string comparisons:
attrs["libname"] != "Browser"and"secret" in attrs["kwname"].lower(), plusattrs["kwname"] in ["Take Screenshot", "Get Page Source"]for the muting._start_keywordfires for every keyword in the run, including keywords of other libraries, so these comparisons were the only thing keeping the hooks from acting on foreign keywords — and they fail in ordinary situations.Both decisions now happen in
run_keyword, which by construction only sees this library's keywords, and they are based on the resolved Python function instead of the displayed keyword name. All threelibnamecomparisons are gone rather than repaired.The listener keeps exactly what genuinely applies to every keyword:
add_context_and_keyword_call_stack_to_tracereplays the whole parent chain when a tracing context opens mid-keyword, andTracingGroupMode.Fullis defined as "also keywords that are not Browser keywords"TracingGroupMode.Full_start_test,_end_test,_start_suiteand_end_suitewere reviewed as well. They are bound to the run rather than to a keyword and must run even when a suite contains no Browser keyword at all, so nothing moves out of them.Issues
Fixes #5154 — the banner is a no-op under
Library Browser AS PW. The subclass half of that report is not addressed, since subclassingBrowseris not a supported import form.Fixes #5169 — muting and secret suppression are disabled for translated keyword names.
Fixes #5170 — the banner paints a resolved secret into the page before
Fill Secretrejects it.Fixes #5172 —
Create CredentialpaintsprivateKey/publicKeyinto the banner, found while responding to the Copilot review below.Behaviour changes worth noting in the release notes
The banner and the secret suppression now work for aliased imports.
Plugin keywords that take a secret are protected for the first time. The banner itself already reached them, because a plugin keyword reports the Browser library as its owner, so
libnamematched. The secret detection did not: it compared the keyword name against the word secret, so a plugin keyword taking a secret under a name that does not contain that word had no protection at all, and its secret argument was resolved into the banner. Detection now reads the argument specification.Plugin Login With CredentialsinExamplePlugin.pycovers this end to end.A failing secret keyword is no longer completely silent. Robot Framework emits its own
Argumentstrace and the keyword failure outsiderun_keyword, so those are no longer suppressed. Only what the library logs during the keyword body still is. Measured on the same failingFill Secretat--loglevel TRACE:TRACE Arguments: [ 'css=input#does-not-exist' | '$PWD' ],FAIL TimeoutError,DEBUG TracebackThe secret value appears in neither, in
output.xmlor inlog.html. #5154 noted the swallowed traceback as a downside of the old suppression, so this reads as an improvement, but it is a visible change.Secret arguments are masked in the banner. An argument is treated as secret when it is named
secretor annotated with Robot Framework'sSecrettype, so this also coverssecret=${PASSWORD},Create Credential'sprivateKey/publicKey, and any future keyword with either shape.Variables are no longer resolved in the banner for a keyword that takes a secret. Masking maps source arguments onto parameters by position and by name, which a collection expansion defeats:
&{arguments}is one source cell carrying several arguments, so it cannot be mapped, and resolving it printed the whole collection into the page. Not resolving at all for these keywords removes the class rather than the two known forms. The trade-off is that a selector passed as a variable to a secret keyword is shown unresolved. Every other keyword resolves its variables exactly as before, guarded by its own test.Nested secret keywords no longer lose the original log level; the suppression counts its depth instead of overwriting a single slot. This is hardening — the only way to reach it today is a
run_on_failurekeyword that is itself a secret keyword.Response to the Copilot review
Two comments came in on this PR. Both are addressed in 6911528, and the full reasoning is in the inline replies:
Fill Secret &{arguments}/@{arguments}): confirmed, reproduced with a real secret value ending up in the banner. Masking maps source cells onto parameters by position and by name, and a collection expansion is one cell carrying several arguments, so it cannot be mapped. Fixed by not resolving variables at all for a keyword that takes a secret, which removes the class instead of the two known forms. Two new acceptance cases cover the dictionary and list forms, plus one proving keywords without a secret still resolve normally.Plugin Login With CredentialsinExamplePlugin.py, deliberately named to not contain that word, covers this end to end, plus four unit tests against a real plugin fixture.Implementation notes
KeywordCallStackEntrygainedkwnameandargs. The banner has to show the call as it was written in the source:run_keywordreceives already converted arguments, wherevalidatehas becomeAssertionOperator.validateand1200anint, which would have brokenkeyword_banner.robot. The four places that hand an entry to a trace group go through_trace_group_arguments._is_secret_keywordis asked on every keyword call, so its answer is cached per keyword name.Verification
inv utest— 409 passed, 2 skipped, including 34 new tests inutest/test_keyword_hooks.pythat were red before the changeinv atest-roboton chromium — 918 tests, 917 passed, 0 failed, 1 skippedatest/test/01_Browser_Management/keyword_hooks.roboton chromium and firefox — 10/10, six of them red before the change, including the ones that caught the plaintext secret in the pageatest/test/09_Plugins/plugin.robot— 9/9; the new plugin case fails againstmainwith the plugin secret painted into the bannerinv lint-robot,inv lint-node, andruff check/ruff formaton every changed file — cleanWebkit could not be verified locally: a minimal script with a plain
Library Browserimport and unmodified code cannot load any page of the test app on this machine, overlocalhostor127.0.0.1, even with a 45 second timeout. CI runs the acceptance tests on chromium.The new suite reads the banner out of the injected
style#kwCallBannerelement instead of the computedbody::beforestyle, so unlike the existingkeyword_banner.robotit is not chromium-only.🤖 Generated with Claude Code
https://claude.ai/code/session_01SuiwgM8Rt9kd6K7TBtFMDz