Properly display non-Polykey errors instead of printing undefined

[aryanjassal]

Description

In Polykey, any non-Polykey errors get serialised as raw JSON, and are not able to be properly converted back into the original errors, or be printed on the output.

The only output we get if a non-Polykey error propagates through is undefined in the output with a non-zero exit code.

This PR should combat this by wrapping it in ErrorPolykeyCLIUnexpectedError, then rendering that error instead. In the future, this should help with identifying exactly what went wrong by providing more context instead of printing a useless undefined as the error message.

As #315 is a quick fix, I will make that change here too.

Issues Fixed

• Fixes Some secrets commands throw an undefined error #311 (REF ENG-434)
• Fixes secrets edit should maintain the websocket as long as the editor session is alive #315 (REF ENG-440)

Tasks

• 1. Wrap the malformed error
• 2. Print the wrapped error using the errorFormatter
• 3. Allow webstream to keep pinging in the background (exec async)
• 4. Run all tests to ensure no regression

Final checklist

• Domain specific tests
• Full tests
• Updated inline-comment documentation
• Lint fixed
• Squash and rebased
• Sanity check the final build

[linear]

ENG-434 Some `secrets` commands throw an `undefined` error

[linear]

ENG-440 `secrets edit` should maintain the websocket as long as the editor session is alive

[aryanjassal]

A separate test is needed to ensure keeping the secrets edit open for around 60 seconds should not cause a websocket connection error.

Something like this can be used:

test(
  'timed test', 
  async () => {
    // launch the editor
    // sleep for 60 seconds
    // close the editor
  },
  timeout=70000
);

[tegefaulkes]

We want to avoid using sleeps in tests generally now. #38

If you can, use promises to wait for the timeout to happen before proceeding.

[aryanjassal]

We want to avoid using sleeps in tests generally now. #38

If you can, use promises to wait for the timeout to happen before proceeding.

Well the sleep won't be in the actual TS test. Instead, the sleep will be inside the command which will be executed by the command as the EDITOR alias. This will simulate the editor being open for a duration without the JS/TS being blocked by a sleep.

Is this also troublesome, or would this be fine?

[tegefaulkes]

Its better if we don't. Just have the editor scrip wait for an input on stdin. We can use that as a semaphore for the next step and then control timing from the test. Also look into using pkExpect for executing it.

[CMCDragonkai]

Never rely on sleep. It's non-deterministic.

[aryanjassal]

Currently, I am wrapping any unexpected errors in ErrorPolykeyCLIUnexpectedError with the actual offending error as the data, and then rendering the error as usual. This probably isn't the best way to do this, so I need some feedback on this.

This is the only real major change being done, so once this is addressed, then we should be able to get this merged pretty quickly.

[aryanjassal]

Currently, the errors look like this. The code for determining the cause string was taken from MatrixAI/js-rpc#69. This is what the undefined errors would look like now.

Note that for testing, I am inducing an undefined error as such: throw 'testing'

-> npm run polykey -- secrets write vault:test

> [email protected] polykey
> ts-node src/polykey.ts secrets write vault:test

z
ErrorPolykeyCLIUnexpectedError: Unexpected error - An unexpected error occurred
  timestamp	Mon Nov 11 2024 12:12:18 GMT+1100 (Australian Eastern Daylight Time)
  cause: Literal: testing

[CMCDragonkai]

Currently, the errors look like this. The code for determining the cause string was taken from MatrixAI/js-rpc#69. This is what the undefined errors would look like now.

Note that for testing, I am inducing an undefined error as such: throw 'testing'

-> npm run polykey -- secrets write vault:test

> [email protected] polykey
> ts-node src/polykey.ts secrets write vault:test

z
ErrorPolykeyCLIUnexpectedError: Unexpected error - An unexpected error occurred
  timestamp	Mon Nov 11 2024 12:12:18 GMT+1100 (Australian Eastern Daylight Time)
  cause: Literal: testing

Why do we need a Literal: prefix? Usually the literal value is a valid cause.

[aryanjassal]

Why do we need a Literal: prefix? Usually the literal value is a valid cause.

I have done this to be consistent and clear. If we throw an error like throw 'ReadableStream', then it is difficult to differentiate a string literal from an object. This is needed as we can throw anything, including objects, in TS.

With the prefix, it is much clearer:

ErrorPolykeyCLIUnexpectedError: Unexpected error - An unexpected error occurred
  timestamp	Mon Nov 11 2024 12:12:18 GMT+1100 (Australian Eastern Daylight Time)
  cause: Literal: ReadableStream

and

ErrorPolykeyCLIUnexpectedError: Unexpected error - An unexpected error occurred
  timestamp	Mon Nov 11 2024 12:12:18 GMT+1100 (Australian Eastern Daylight Time)
  cause: Object: ReadableStream

[aryanjassal]

Just some key points I wanted to mention.

[aryanjassal]

All the tasks have been completed and the CI is also passing. Merging as completed.

[CMCDragonkai]

Remember errors have the name, description and message. The message is instance context while description is static.