view release on metacpan or  search on metacpan

TODO.md  view on Meta::CPAN

# TODO

## Baseline code review

No full code review has ever been done: the first one covered only the
`main...moo` diff (Moo and Ouch), and the security review only looked for
ways to abuse the code, not for correctness. To stay within usage limits,
the review is split in chunks, one per session, in order of priority.

For each chunk:

- start a new session (or `/clear`) first;
- run `/code-review <level>`, naming the files in scope and saying that
  other files may be read for context but only the chunk is reported on,
  e.g. `/code-review high` "review lib/AWS/Signature/V4/Chunker.pm,
  Checksum.pm and their tests t/streaming.t, t/trailer.t; read other files
  only for context";
- have the findings saved to `tmp/review-<n>-<name>.md` (`tmp/` is
  git-ignored), so that nothing is lost between sessions;
- fix after each chunk, or collect all the findings first and fix them
  together.

Chunks 1 to 3 cover all the code: stopping after them already gives a
baseline for everything that runs.

- [x] **1. Signing core** (level `max`, ~890 lines):
      `lib/AWS/Signature/V4.pm` with `t/canonical.t`, `t/basic.t`,
      `t/presign.t`. Canonical request, encoding, presign,
      `encoded_length`.
- [x] **2. Streaming** (level `high`, ~440 lines):
      `lib/AWS/Signature/V4/Chunker.pm`, `Checksum.pm` with
      `t/streaming.t`, `t/trailer.t`.
- [x] **3. Variants and errors** (level `high`, ~760 lines):
      `lib/AWS/Signature/V4/X509.pm`, `Credentials.pm`, `Error.pm` with
      `t/chain.t`, `t/variants.t`, `t/error.t`, `t/errors.t`,
      `t/security.t`. DER parsing, key loading.
- [x] **4. Documentation** (level `medium`, ~1140 lines): all the `.pod`
      files, checked against the code: do they promise anything that the
      code does not do?
- [x] **5. Examples** (level `medium` or `low`, ~670 lines): `eg/*.pl`,
      `eg/README.md`. Lowest priority, can be skipped.

## Fixing the review findings

The findings are in `tmp/review-1-signing-core.md` to
`tmp/review-5-examples.md`. Fix them one file at a time, in the same order
as the review: later areas depend on earlier ones, and a fix in the code
often needs matching changes in the POD, the tests and `eg/`.

Model: Opus 5 at high effort (or xhigh) by default. Switch with `/model`
before a chunk when useful:

- Sonnet 5 is enough for chunks 4 and 5 (wording, error checks,
  comments), at a lower cost;
- Fable 5.1 (twice the cost of Opus 5) only if a fix in chunk 1 turns out
  to be hard and Opus's first attempt does not hold up;
- not Haiku: too easy to miss something in signing code.

For each chunk:

- start a new session (or `/clear`) first;
- ask to fix the findings in `tmp/review-<n>-<name>.md`, updating the
  POD, tests and examples affected by each fix;
- run the test suite and make sure it passes. The dependencies are
  installed in `local/` (by `carton`), so they have to be added to the
  include path: `prove -l -Ilocal/lib/perl5 t/`, or `carton exec prove -l
  t/`. Plain `prove -l t/` dies on `Can't locate Moo.pm`;
- make one commit per chunk.

- [x] **1. Signing core** (Opus 5, Fable 5.1 if needed):
      `tmp/review-1-signing-core.md`.
- [x] **2. Streaming** (Opus 5): `tmp/review-2-streaming.md`.
- [x] **3. Variants and errors** (Opus 5):
      `tmp/review-3-variants-errors.md`.
- [x] **4. Documentation** (Sonnet 5 or Opus 5):
      `tmp/review-4-documentation.md`.
- [x] **5. Examples** (Sonnet 5 or Opus 5): `tmp/review-5-examples.md`.
      Start with the medium finding in `eg/05-s3-chunked-upload.pl`.
- [x] **6. Final check** (Opus 5): run `/code-review high` on the whole
      diff of the fixes (from `d7b121f`, the last commit before them) to
      catch regressions the fixes introduced.
      `tmp/review-6-final.md`, fixed in `ed8fd99`.

## Before release

- [x] **Confirm the `Content-Encoding` order against a real bucket.**
      Done: S3 in `eu-south-1` accepted it, and the stored metadata shows
      the list was parsed rather than merely tolerated. The control, which
      sent `aws-chunked` alone, came back stored as no `Content-Encoding`
      at all; the real case, which sent `gzip,aws-chunked`, came back
      stored as `gzip`. So S3 strips the `aws-chunked` token from the end
      of the list and keeps what is left, which is what RFC 9110 and
      botocore describe. Both objects round-tripped byte-identical.

      What follows is the question as it stood, kept because it explains
      why the module emits this order and how to ask again if AWS ever
      answers differently.

      With `streaming` and an encoding already set by the caller, `sign`
      emits `gzip,aws-chunked` (`lib/AWS/Signature/V4.pm`, in the
      `$streaming` branch; asserted in `t/streaming.t` and documented in
      `V4.pod` under `sign`). This was the only behaviour in the module
      chosen from indirect evidence rather than from AWS itself, because
      the authorities contradict each other:

      - RFC 9110 §8.4 wants the encodings in the order they were applied,
        and `aws-chunked` is the outermost one, applied to the already
        compressed data: `gzip,aws-chunked`;
      - botocore (`httpchecksum.py`) agrees, appending: `headers
        ["Content-Encoding"] += ",aws-chunked"`;
      - the first review cited the S3 documentation for the opposite
        order, and a boto3 issue reports seeing `aws-chunked,gzip` in
        stored metadata.

      The guess was that S3 strips the `aws-chunked` token from either
      position, so that both orders would be accepted and the point would
      be moot; the run above confirms the stripping, at least from the
      end. It does not say what the other order would do, and there is no
      reason to find out while the module sends this one.

      `eg/10-s3-content-encoding-probe.pl` settled it. It uploads a
      gzipped object with `streaming`, plus a control without any
      `Content-Encoding` so that a rejection can be told apart from a
      wrong bucket, region or set of credentials; it reads both back,
      checks the bytes round-trip, and deletes them again. Run it against
      a bucket that can be written to:

      ```shell
      AWS_REGION=eu-west-1 \
         AWS_ACCESS_KEY_ID=... AWS_SECRET_ACCESS_KEY=... \
         ./eg/10-s3-content-encoding-probe.pl my-bucket
      ```

      It exits zero and prints `verdict: S3 accepted gzip,aws-chunked`