-
-
Notifications
You must be signed in to change notification settings - Fork 4
fix_container_permissions: rewrite to use mwscript #205
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
Universal-Omega
wants to merge
3
commits into
main
Choose a base branch
from
fix_container_permissions-mwscript
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Repository: miraheze/python-functions
Length of output: 1642
🏁 Script executed:
Repository: miraheze/python-functions
Length of output: 51
🏁 Script executed:
Repository: miraheze/python-functions
Length of output: 1845
🏁 Script executed:
cat -n miraheze/mediawiki/mwscript.py | head -50Repository: miraheze/python-functions
Length of output: 1447
🏁 Script executed:
Repository: miraheze/python-functions
Length of output: 1584
🏁 Script executed:
cat -n miraheze/mediawiki/mwimport.py | head -70Repository: miraheze/python-functions
Length of output: 3758
🏁 Script executed:
Repository: miraheze/python-functions
Length of output: 864
🏁 Script executed:
rg "mwscript|swift" --type py -B 2 -A 2Repository: miraheze/python-functions
Length of output: 16389
🏁 Script executed:
Repository: miraheze/python-functions
Length of output: 1314
🏁 Script executed:
Repository: miraheze/python-functions
Length of output: 2849
🏁 Script executed:
Repository: miraheze/python-functions
Length of output: 80
🏁 Script executed:
Repository: miraheze/python-functions
Length of output: 1341
🏁 Script executed:
Repository: miraheze/python-functions
Length of output: 147
Use absolute paths or document PATH requirements for
mwscriptandswift.Bare command names are vulnerable to PATH hijacking. Either resolve to absolute paths (preferred per codebase pattern) or document that these commands must be available in PATH. Avoid module-level initialisation that breaks imports if executables are unavailable.
🧰 Tools
🪛 Ruff (0.14.13)
7-7:
subprocesscall: check for execution of untrusted input(S603)
8-8: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mwscript is a function available within the package, you could just call it as a function. That would probably be cleaner.
We can assume that Swift is on the PATH when running a Swift maintenance script so I think that element is safe in this context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Length of output: 6175
@RhinosF1, you're absolutely right about using the mwscript function from the package - that's a much cleaner approach!Looking at the
mwscript.pycode, I can see the challenge: the current implementation needs to capture stdout/stderr to parse for failed containers, butmwscript.run()usesos.system()which doesn't support output capture.Here's a suggested approach that leverages the mwscript module while still capturing output:
This approach:
mwscript.get_commands()to build the proper command (handling versioning, sudo, etc.)Alternatively, if you'd prefer to enhance
mwscript.run()to support output capture mode, that would benefit other use cases too.Regarding Swift - acknowledged that it's safe to assume on PATH in this maintenance script context. ✓
🧬 Code Graph Analysis Results
[miraheze/swift/fix_container_permissions.py:6-12]
def run_mwscript_setcontainersaccess(wiki: str, check: bool) -> subprocess.CompletedProcess[str]:
return subprocess.run(
['mwscript', 'CreateWiki:SetContainersAccess', wiki, '--yes'],
capture_output=True,
text=True,
check=check,
)
[miraheze/swift/fix_container_permissions.py:15-34]
def fix_container_perms(wiki: str) -> None:
out = run_mwscript_setcontainersaccess(wiki, check=False)
matches = re.findall(
r"Making sure 'mwstore://miraheze-swift/([^']+)' [^\n]+.failed.",
(out.stdout or '') + '\n' + (out.stderr or ''),
)
[miraheze/swift/fix_container_permissions.py:37-46]
def main() -> None:
parser = argparse.ArgumentParser(description='Fix container permissions for a specified wiki')
parser.add_argument('--wiki', required=True, help='wiki database name')
[miraheze/swift/fix_container_permissions.py:45-45]
if name == 'main':
main()
✏️ Learnings added
🧠 Learnings used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updating mwscript to use subprocess.run() is acceptable to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@RhinosF1, excellent! Updatingmwscriptto usesubprocess.run()will make it much more flexible and allow for proper output capture across all scripts that need it.Would you like me to help plan the implementation? I can:
mwscript.run()to usesubprocess.run()instead ofos.system(), adding a parameter to control whether output should be capturedfix_container_permissions.pyto import and use the enhancedmwscriptfunction directlymwscript.run()This refactor would benefit not just this script, but any future scripts that need to capture MediaWiki maintenance script output.
Shall I proceed with planning these changes?
🧠 Learnings used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
@RhinosF1, great! Let me examine the current implementation and existing usages to ensure we maintain backwards compatibility while adding the new functionality.🏁 Script executed:
Length of output: 6014
@RhinosF1, perfect! Based on my analysis, I can see thatmwscript.run()currently usesos.system()and there are no existing usages in the codebase (except tests), which makes this refactor quite safe. Let me plan the implementation:Task: Refactor mwscript to use subprocess.run() and update fix_container_permissions.py
Changes overview:
mwscript.run()to usesubprocess.run()with optional output capturefix_container_permissions.pyto import and use mwscript module functionsBenefits:
This refactor will make the code cleaner, more maintainable, and provide the output capture functionality needed for
fix_container_permissions.pywhile preserving all existing behavior for other scripts that might use mwscript in the future.🧬 Code Graph Analysis Results
🧠 Learnings used
💡 For code changes across multiple files, use
@coderabbitaiplan in an issue comment