Skip to content

fix(ses,lockdown): make filenames in stacktraces clickable #2747

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

Merged
merged 2 commits into from
May 17, 2025

Conversation

erights
Copy link
Contributor

@erights erights commented Mar 22, 2025

Refs: #2359

Description

Added two new stackFiltering: options

  • 'omit-frames' -- Only omit likely uninteresting frames. Keep original paths.
  • 'shorten-paths' -- Only shorten paths to text likely clickable in an IDE

This fills out the matrix of what should have been orthogonal options.
The existing 'concise' setting both omits likely uninteresting frames and
shortens their paths. The existing 'verbose' setting does neither.

The path shortening caused by either 'concise' or 'shorten-paths' is also enhanced with two new transformations, in order to make these paths easily and usefully clickable in an IDE like vscode.

  • We would already shorten, for example

    'Object.bar (/vat-v1/.../eventual-send/test/deep-send.test.js:13:21)'
    

    to

    'Object.bar (eventual-send/test/deep-send.test.js:13:21)'.
    

    As of this PR, we would also shorten, for example

    'Object.bar (.../eventual-send/test/deep-send.test.js:13:21)'
    

    to

    'Object.bar (eventual-send/test/deep-send.test.js:13:21)'.
    
  • As of this PR, we would would shorten, for example

    'Object.bar (file://test/deep-send.test.js:13:21)'
    

    to

    'Object.bar (test/deep-send.test.js:13:21)'.
    

    because the shorter string is a valid relative path, which is therefore usefully clickable.
    But we would still not shorten or transform, for example

    'file:///Users/markmiller/src/ongithub/endojs/endo/packages/eventual-send/test/deep-send.test.js:13:21'
    

    because three slashes are normally followed by an absolute path, which is therefore usefully clickable.

  • This PR changes commit-debug.js from 'errorTaming: 'unsafe-debug' to errorTaming: 'unsafe' so that exiting programs whose initialization goes through commit-debug.js can benefit from stackFiltering: at all, as well as these new enhancements. TODO but this PR must also fix, by other means, the typescript line number bug that motivated 'unsafe-debug' in the first place. See fix(lockdown): change commit-debug.js to unsafe-debug #2359

    • Manually verified that the typescript bug that motivated 'unsafe-debug' is now fixed, and so 'unsafe-debug' is no longer necessary. Presumably this is because we moved to erasable-typescript which does not change line and column numbers.
  • This PR does not change that all this stackFiltering: logic is specific to v8. We should move and generalize this code so that it applies to all relevant JS platforms. TODO at least file an issue for that.

Security Considerations

none

Scaling Considerations

none

Documentation Considerations

Wherever we document the stackFiltering: rules, we should update those. This PR already updates such documentation within agoric-sdk itself.

Testing Considerations

stackFiltering: is hard to automatically test because there is no specified or portable behavior for the platform-provided stacks. Instead, we just have manual inspection tests, where you run it and visually sanity check the output. I have added such checks for the new options setting and manually sanity checked them.

Compatibility Considerations

The only observable change to existing programs is the more aggressive path shortening explained above. I doubt this will cause any compat problems, especially since the platform stack traces are unspecified anyway.

Upgrade Considerations

None.

  • Update NEWS.md for user-facing changes.

@erights erights self-assigned this Mar 22, 2025
@erights erights changed the title Markm clickable stack lines fix(ses,lockdown): make filenames in stacktraces clickable Mar 22, 2025
@erights erights changed the base branch from master to markm-getenv-detects-more-errors March 22, 2025 21:38
@erights erights force-pushed the markm-clickable-stack-lines branch from 7465c6a to 42ac958 Compare March 22, 2025 23:10
@erights erights force-pushed the markm-getenv-detects-more-errors branch from d4c1a93 to 4f519e4 Compare March 24, 2025 23:44
@erights erights force-pushed the markm-clickable-stack-lines branch from 42ac958 to e7158a9 Compare March 24, 2025 23:47
@erights erights requested a review from kriskowal March 26, 2025 23:26
@erights erights force-pushed the markm-getenv-detects-more-errors branch from 4f519e4 to f98efc0 Compare April 5, 2025 21:42
@erights erights force-pushed the markm-clickable-stack-lines branch from cdc3255 to 4520477 Compare April 5, 2025 21:45
@erights erights force-pushed the markm-getenv-detects-more-errors branch from f98efc0 to e7f81bd Compare April 8, 2025 00:11
@erights erights force-pushed the markm-clickable-stack-lines branch from 4520477 to 399cf46 Compare April 8, 2025 00:11
@erights erights force-pushed the markm-getenv-detects-more-errors branch from e7f81bd to fda2e75 Compare May 12, 2025 19:45
@erights erights force-pushed the markm-clickable-stack-lines branch from 399cf46 to abdd6fc Compare May 12, 2025 19:46
@boneskull
Copy link
Member

👍 for clickable files

Base automatically changed from markm-getenv-detects-more-errors to master May 13, 2025 01:13
@erights erights force-pushed the markm-clickable-stack-lines branch from abdd6fc to f388379 Compare May 13, 2025 02:17
@erights erights requested a review from boneskull May 13, 2025 02:17
@erights erights marked this pull request as ready for review May 13, 2025 02:30
@erights erights force-pushed the markm-clickable-stack-lines branch 3 times, most recently from 544a597 to 6a34c19 Compare May 15, 2025 23:15
@erights erights force-pushed the markm-clickable-stack-lines branch from 6a34c19 to b130151 Compare May 17, 2025 00:40
@erights erights merged commit 178e253 into master May 17, 2025
16 checks passed
@erights erights deleted the markm-clickable-stack-lines branch May 17, 2025 01:28
boneskull pushed a commit that referenced this pull request Jun 4, 2025
Refs: #2359 

## Description

Added two new `stackFiltering:` options
- `'omit-frames'` -- Only omit likely uninteresting frames. Keep
original paths.
- `'shorten-paths'` -- Only shorten paths to text likely clickable in an
IDE

  This fills out the matrix of what should have been orthogonal options.
The existing `'concise'` setting both omits likely uninteresting frames
and
  shortens their paths. The existing `'verbose'` setting does neither.

The path shortening caused by either `'concise'` or `'shorten-paths'` is
also enhanced with two new transformations, in order to make these paths
easily and usefully clickable in an IDE like vscode.

- We would already shorten, for example
   ```
   'Object.bar (/vat-v1/.../eventual-send/test/deep-send.test.js:13:21)'
   ```
   to
   ```
   'Object.bar (eventual-send/test/deep-send.test.js:13:21)'.
   ```
   As of this PR, we would also shorten, for example
   ```
   'Object.bar (.../eventual-send/test/deep-send.test.js:13:21)'
   ```
   to
   ```
   'Object.bar (eventual-send/test/deep-send.test.js:13:21)'.
   ```
- As of this PR, we would would shorten, for example
   ```
   'Object.bar (file://test/deep-send.test.js:13:21)'
   ```
   to
   ```
   'Object.bar (test/deep-send.test.js:13:21)'.
   ```
because the shorter string is a valid relative path, which is therefore
usefully clickable.
   But we would still not shorten or transform, for example
   ```

'file:///Users/markmiller/src/ongithub/endojs/endo/packages/eventual-send/test/deep-send.test.js:13:21'
   ```
because three slashes are normally followed by an absolute path, which
is therefore usefully clickable.

- [x] This PR changes commit-debug.js from `'errorTaming:
'unsafe-debug'` to `errorTaming: 'unsafe'` so that exiting programs
whose initialization goes through `commit-debug.js` can benefit from
`stackFiltering:` at all, as well as these new enhancements. TODO but
this PR must also fix, by other means, the typescript line number bug
that motivated `'unsafe-debug'` in the first place. See #2359
- Manually verified that the typescript bug that motivated
`'unsafe-debug'` is now fixed, and so `'unsafe-debug'` is no longer
necessary. Presumably this is because we moved to erasable-typescript
which does not change line and column numbers.

- [ ] This PR does not change that all this `stackFiltering:` logic is
specific to v8. We should move and generalize this code so that it
applies to all relevant JS platforms. TODO at least file an issue for
that.

### Security Considerations

none
### Scaling Considerations

none
### Documentation Considerations

Wherever we document the `stackFiltering:` rules, we should update
those. This PR already updates such documentation within agoric-sdk
itself.

### Testing Considerations

`stackFiltering:` is hard to automatically test because there is no
specified or portable behavior for the platform-provided stacks.
Instead, we just have manual inspection tests, where you run it and
visually sanity check the output. I have added such checks for the new
options setting and manually sanity checked them.

### Compatibility Considerations

The only observable change to existing programs is the more aggressive
path shortening explained above. I doubt this will cause any compat
problems, especially since the platform stack traces are unspecified
anyway.

### Upgrade Considerations

None.

- [x] Update `NEWS.md` for user-facing 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.

2 participants